build: enforce golden files and lint entry-point configuration #18092
Conversation
googlebot
added
the
cla: yes
devversion
force-pushed
the
build/enforce-goldens-for-configured-ep
branch
2 times, most recently
from
521cc85 to
4d83d97
Compare
devversion
force-pushed
the
build/enforce-goldens-for-configured-ep
branch
from
4d83d97 to
477343b
Compare
devversion
added
P3
|
@mmalerba I'm adding the |
| @@ -28,14 +28,15 @@ | |||
| "gulp": "gulp", | |||
| "stage-release": "ts-node --project tools/release/ tools/release/stage-release.ts", | |||
| "publish-release": "ts-node --project tools/release/ tools/release/publish-release.ts", | |||
| "check-entry-point-setup": "node ./scripts/check-entry-point-setup.js", | |||
There was a problem hiding this comment.
Couldn't we run this with ts-node and have it be in TypeScript like we have for the rest of the scripts?
There was a problem hiding this comment.
I decided to not use TypeScript for the script so that it could be simply run without ts-node. I did the same for build-packages-dist.js and deploy-dev-app.js. I'm not sure what the ideal language is for these scripts. I found everything needed for the scripts in standard JS and it doesn't require the slower ts-node.
There was a problem hiding this comment.
It's not much slower though and most of the other tooling is in TS already. E.g. check the rest of the scripts around it in the package.json.
There was a problem hiding this comment.
The majority of Node scripts in scripts/ is JavaScript already. Other tools linked in package.json like the release scripts are Typescript-based since they are split up in multiple files and they are not really scripts, but rather tools/. Also most of these TS scripts do not leverage TS specific features, so they are technically JS already (maybe a few types)
For me personally, the scripts in ./scripts/ should primarily be about usability and not need a preprocessor (unless there is a reason it's required for a particular script). Re slowness: I tried running a simple script and for the simple script I hit quite a difference already.
| ts-node | node |
|---|---|
| real 0m1.380s user 0m0.030s sys 0m0.075s |
real 0m0.073s user 0m0.000s sys 0m0.015s |
I don't mind too much. My point is just that the majority of Node scripts in ./scripts is already JavaScript based, so we should rather decide on one language for these scripts. This seems out of scope for this PR though?
There was a problem hiding this comment.
I'm not arguing that it's slower, but it's not something we run manually ourselves and one or two extra seconds is nothing for the build which is taking minutes. I don't mind it either way. We can see what others think about it.
There was a problem hiding this comment.
The scripts are not necessarily about the build. We could say that the time difference is not impacting much since scripts like check-rollup-globals run on CI and the actual JS code is fast.
What's the benefit of using TypeScript for scripts which do not use any TS features? I could imagine consistency (which is good). As said above, I don't mind too much either, so if we decide to do it for consistency, then I'm happy to switch all other scripts to TS (it might be good to think about the Bash scripts too)
devversion
force-pushed
the
build/enforce-goldens-for-configured-ep
branch
from
477343b to
00913cc
Compare
|
awesome, thank you! |
…ar#18092) * build: enforce golden files for configured entry points * build: add linting to ensure entry-points are configured
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
public-api.tsfiles.Resolves COMP-263