Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ci): remove need to compile scripts #5370

Merged
merged 6 commits into from
May 28, 2024

Conversation

christian-bromann
Copy link
Member

@christian-bromann christian-bromann commented Feb 15, 2024

What is the current behavior?

Currently a developer working on Stencil needs to compile the entire scripts folder first before running the files in them. This seems cumbersome and there is an opportunity to improve the DX here.

What is the new behavior?

By adding tsx we can get rid of the intermediate build step by running TS files on demand. This simplifies a lot of internal scripts and confusion when it comes to resolving paths.

Documentation

Does this introduce a breaking change?

  • Yes
  • No

Testing

Checkout the branch and run npm run build.

Other information

n/a

Copy link
Contributor

github-actions bot commented Feb 15, 2024

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/9270562098/artifacts/1544405233

If your browser saves files to ~/Downloads you can install it like so:

unzip -d ~/Downloads ~/Downloads/stencil-core-4.18.2-dev.1716904125.f0e8bf4.tgz.zip && npm install ~/Downloads/stencil-core-4.18.2-dev.1716904125.f0e8bf4.tgz

@tanner-reits tanner-reits removed their request for review May 2, 2024 19:39
@christian-bromann
Copy link
Member Author

Tech Debt Burndown / Download error files and report (pull_request_target) Failing after 31s

This is due to the fact that we are running the workflow from main which still uses the compiled script.

@christian-bromann christian-bromann marked this pull request as ready for review May 17, 2024 00:19
@christian-bromann
Copy link
Member Author

@tanner-reits @alicewriteswrongs this is ready for review now.

rollup.config.js Outdated Show resolved Hide resolved
@@ -122,6 +119,7 @@
"rollup-plugin-sourcemaps": "^0.6.3",
"semver": "^7.3.7",
"terser": "5.31.0",
"tsx": "^4.10.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reasoning on choosing tsx over ts-node? Just curious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tsx is much better maintained as ts-node. I am switching in all my projects to tsx now.

@alicewriteswrongs
Copy link
Member

Definitely a nicer DX here! It's a bit less mysterious why we're doing what we doing for the build here I think.

One thing I'm wondering about is type checking for the scripts/ directory. In our current setup on main we run a full typescript build of the directory which means that we'll never end up running the scripts if there's a type error. From what I can tell tsx takes the stance as a project that type checking in e.g. your editor is good enough and does not do any type checking when you go to execute a file (I confirmed that by putting const stringy: string = 1; into scripts/index.ts and I was able to run it with no error).

Anywhooo - just wondering if this is the direction we want to go, since it does open up the possibility of some type errors slipping through in for instance the release scripts and then only showing up as runtime error (or wrong behavior!) when we run on CI.

The tsx docs suggest possibly doing a tsc --noEmit before you run the scripts - does that make sense here?

@christian-bromann
Copy link
Member Author

@alicewriteswrongs good catch 👌 I didn't know that! I pushed a change to include type checks as recommend by the tsx docs.

package.json Outdated
"tsc.prod": "tsc",
"tsc.scripts": "tsc --build --force scripts/tsconfig.json",
"tsc.watch": "tsc --incremental --watch"
"ts": "tsc --noEmit --project scripts/tsconfig.json && tsx",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a nice way to do this 👍

package.json Outdated
Comment on lines 39 to 40
"rollup.prod.ci": "rollup --config --config-prod --config-ci",
"rollup.watch": "rollup --watch --config",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops guess those snuck through in #5698 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😉

Copy link
Member

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI error, but otherwise looks good

@christian-bromann
Copy link
Member Author

CI error, but otherwise looks good

This is due to the fact that we are running the workflow from main which still uses the compiled script. Therefore I suggest to suggest merging by bypassing the protection.

@christian-bromann christian-bromann merged commit a40cee1 into main May 28, 2024
92 of 93 checks passed
@christian-bromann christian-bromann deleted the cb/remove-scripts-build branch May 28, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants