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

fix: ES2022 build compatibility and move to NodeNext for module #2258

Conversation

vladfrangu
Copy link
Member

Closes #2257

@github-actions github-actions bot added this to the 79th sprint - Tooling team milestone Dec 27, 2023
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Dec 27, 2023
@@ -1,7 +1,8 @@
{
"extends": "@apify/tsconfig",
"compilerOptions": {
"module": "ES2022",
"module": "NodeNext",
"moduleResolution": "NodeNext",
Copy link
Member

Choose a reason for hiding this comment

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

Afaik this is implicit with module: nodenext. Also, isnt this breaking?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not, if we specify module, it'll throw unless we also specify moduleResolution

Its not... breaking per say, although some older modules might throw compile errors when import-ed... But we should be moving away from compiling to ES20XX and compile to Node16/Next anyways (or bring in a bundler that isn't tsc for it)

Copy link
Member

Choose a reason for hiding this comment

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

I actually saw the opposite behaviour in mikroorm, but i think it was a ts bug in validation. But i do remember reading in docs that this is actually inferred (and it does work that way on my end, i get dynamic imports in the compiled files by only specifying module)

Copy link
Member Author

Choose a reason for hiding this comment

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

Funky, I tried changing just module in our templates and then ran tsc and it threw an immediate error that moduleResolution must also be set to it 😅

Copy link
Member

Choose a reason for hiding this comment

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

Well, programming is sometimes weird :) feel free to merge this, even if it would be duplicating things, its fine

@vladfrangu vladfrangu merged commit 7fe1e68 into master Dec 27, 2023
8 checks passed
@vladfrangu vladfrangu deleted the fix/move-templates-to-nodenext-module-and-installing-missing-types-pkg branch December 27, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript builds fail when generated from 'npx crawlee create'
2 participants