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.

Sign up for GitHub

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 typescript from 5.1.6 to 5.2.2 #460

Merged
merged 2 commits into from
Sep 5, 2023
Merged

Bump typescript from 5.1.6 to 5.2.2 #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcd1cd4
Bump typescript from 5.1.6 to 5.2.2
dependabot[bot] Aug 25, 2023
87ad78a
Defer to @tsconfig settings for module
slifty Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"engines": {
"node": "18"
},
"type": "commonjs",
"devDependencies": {
"@tsconfig/node18": "^18.2.1",
"@types/cors": "^2.8.13",
Expand All @@ -66,7 +67,7 @@
"swagger-spec-validator": "^5.0.0",
"ts-jest": "^28.0.5",
"ts-node": "^10.8.1",
"typescript": "^5.1.6"
"typescript": "^5.2.2"
},
"dependencies": {
"ajv": "^8.12.0",
Expand Down
1 change: 0 additions & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{
"extends": "@tsconfig/node18/tsconfig.json",
"compilerOptions": {
"module": "commonjs",
Copy link
Contributor

Choose a reason for hiding this comment

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

For context, as the failing build log will eventually expire, this is the error message:

Error: tsconfig.json(4,15): error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.

This error is new to TypeScript 5.2.2, and comes from this PR: microsoft/TypeScript#54567 .

The relevant bits of configuration from @tsconfig/node18/tsconfig.json:

{
  "compilerOptions": {
    "module": "Node16",
    "moduleResolution": "node16"
  }
}

This was also recently changed: in tsconfig/bases#197, which we brought in via #453 .

I do not yet fully understand the consequences of opting in to node16 for module and moduleResolution (see also the docs for module in tsconfig.json). My concern is that we'd be opening the door to ESM, which I would very much like to avoid.

I'll keep researching, but wanted to document how we got to this question in the first place. Despite being a single line change, this is a complicated situation!

Copy link
Member

Choose a reason for hiding this comment

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

@jasonaowen thanks for doing this research -- once the dust settles I'll make sure to update the commit message with the information (and add you as a co-author)

Copy link
Member

@slifty slifty Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Reading through https://www.typescriptlang.org/docs/handbook/esm-node.html, it looks like Node16 means typescript SUPPORTS ESM, not that it must be ESM. Specifically, Node16 means that if type: 'module' is defined in package.json then typescript will start to flag ESM errors.

This is why the PR does not currently error (we don't specify type in package.json so it is using the default, which is commonjs).

If we add "type": "module" to package.json then running build results in tons of ESM errors:

image

This is corroborated with the TS documentation Which notes:

Available from 4.7+, the node16 and nodenext modes integrate with Node’s native ECMAScript Module support. The emitted JavaScript uses either CommonJS or ES2020 output depending on the file extension and the value of the type setting in the nearest package.json. Module resolution also works differently. You can learn more in the handbook.

Bottom line is I think it's safe to merge this. We might consider also explicitly setting "type": "commonjs" in the package.json.

Copy link
Contributor

@jasonaowen jasonaowen Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you, @slifty! I think that matches my initial research.

  1. does this allow us to use ESM-only libraries? what happens if we do?
  2. are there any drawbacks to setting "type": "commonjs"? if not, I really like that idea!

Copy link
Member

Choose a reason for hiding this comment

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

@jasonaowen

does this allow us to use ESM-only libraries? what happens if we do?

This is a great question! I'll test it out (my theory is no since it has to be able to compile to common js. stay tuned to find out)

are there any drawbacks to setting "type": "commonjs"? if not, I really like that idea!

My understanding of the documentation of type is that omission is the same as explicitly setting to commonjs so let's do it!

Copy link
Member

@slifty slifty Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The behavior on this branch appears to the same as main regarding esm-only modules still error in the same as on main. I tried installing node-fetch on both and in both cases it triggered the same output when running build both for when package.json contained "type": "commonjs" and "type": "module".

This makes sense to me because the way imports are processed is dictated by the moduleResolution setting (as opposed to the module setting). The moduleResolution is the same on this branch and on main (it is Node16`).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for following up on this, @slifty!

This makes sense to me because the way imports are processed is dictated by the moduleResolution setting (as opposed to the module setting). The moduleResolution is the same on this branch and on main (it is Node16).

Note that that changed recently, in #412 - previously the moduleResolution was node, which the TypeScript docs describe as

'node10' (previously called 'node') for Node.js versions older than v10, which only support CommonJS require. You probably won’t need to use node10 in modern code.

I ran through the manual test you described with commit 86ea0c9 (the parent commit of #412), and the new behavior is better: previously, it would successfully build, but then at runtime say

Error [ERR_REQUIRE_ESM]: require() of ES Module node_modules/node-fetch/src/index.js from dist/index.js not supported.
Instead change the require of node_modules/node-fetch/src/index.js in dist/index.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (dist/index.js:8:38) {
  code: 'ERR_REQUIRE_ESM'
}

So, +1 to this change, and thanks again @slifty for digging in!

"outDir": "dist",
"declaration": true,
"esModuleInterop": true,
Expand Down