-
-
Notifications
You must be signed in to change notification settings - Fork 689
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
Bump ts-node to 10.9, allow node 18, fix json import assert #1431
Conversation
Bundle size report: Size Change: 0 B
ℹ️ View Details
|
Overall this is very close, but there is a bit of work left related to the unassert plugin. The inline assert tests take up 5 kb, so if we leave them in production they add that. We can also remove them altogether, or we figure out how fix the unassert plugin. |
I don't see a lot of value in asserting inside the code. The tests should cover the cases where assert is needed in my opinion... |
I actually managed to fix the plugin and forgot about it again. I made an upsteam PR a week ago unassert-js/rollup-plugin-unassert#8, but for now we can just use the fork i made. |
So now I believe we have everything working with node 18 I also think we should remove these asserts at some point, as they are starting to hold us back in different ways. I made a branch a few weeks back called remove-browserify where I commented all the node assert |
node-version: 16 | ||
architecture: x64 | ||
- run: npm ci | ||
- run: npm run typecheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this run tsc on the tests files as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - it didn't. I have just now added a tsconfig.typecheck.json file so we can tailor the typecheck specifically to the areas we want, so now they are covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a lot of dependencies were added... While I believe this is needed we need to think how to reduce those in the future...
Yes, 2 plugins (one depend on the other) to make rollup recognise the import assert. I expect it'll somehow be baked into rollup/typescript quite soon so we can remove them again. |
Compatibility with node 16 and 18 interchangeably.
Changes:
import assert type json
statements.typecheck
command is now package.json instead using tsc, and also a github test runner for the CI/CD. This is actually better as theres no point in running a full typecheck for all 122 unit tests...