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

Bump typescript from 5.1.6 to 5.2.2 #460

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Aug 25, 2023

Bumps typescript from 5.1.6 to 5.2.2.

Release notes

Sourced from typescript's releases.

TypeScript 5.2 RC

For release notes, check out the release announcement.

For the complete list of fixed issues, check out the

Downloads are available on:

TypeScript 5.2 Beta

For release notes, check out the release announcement.

For the complete list of fixed issues, check out the

Downloads are available on npm.

Commits

Dependabot compatibility score

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

@dependabot dependabot bot added dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code labels Aug 25, 2023
@dependabot dependabot bot force-pushed the dependabot/npm_and_yarn/typescript-5.2.2 branch from ca21aa2 to e567137 Compare August 25, 2023 03:39
@slifty slifty requested a review from jasonaowen August 25, 2023 03:50
@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3233434) 95.24% compared to head (87ad78a) 95.24%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #460   +/-   ##
=======================================
  Coverage   95.24%   95.24%           
=======================================
  Files          66       66           
  Lines         968      968           
  Branches      161      161           
=======================================
  Hits          922      922           
  Misses         43       43           
  Partials        3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -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

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

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

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!

@slifty
Copy link
Member

slifty commented Aug 30, 2023

We spoke about this last week -- next step (which I own) is to spend a bit of time researching what exactly the Node16 setting means and if it would cause this project to become ESM, and what that would mean.

@slifty slifty force-pushed the dependabot/npm_and_yarn/typescript-5.2.2 branch from 22f5f52 to b1fd1bf Compare August 31, 2023 20:32
Copy link
Contributor

@jasonaowen jasonaowen left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this, @slifty! I feel like I understand the change now, and this is leaving us in a better state. +1!

@@ -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.

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!

dependabot bot and others added 2 commits September 5, 2023 16:21
Bumps [typescript](https://github.com/Microsoft/TypeScript) from 5.1.6 to 5.2.2.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](https://github.com/Microsoft/TypeScript/commits)

---
updated-dependencies:
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
The "module" setting is defined in the base tsconfig.json that we're
using, and overriding it actually resulted in a mismatch with the
latest `moduleResolution` that was being defined in that base.

The `Node16` setting appears to simply signal *support* for ESM
depending on the `type` setting in `package.json`.  This is based on the
ECMAScript Modules in Node.js documentation[1] as well as the TS
configuration documentation[2].

I've added `"type": "commonjs"` to `package.json` although the omitted
setting defaulted to `commonjs` which means this change is just to make
the setting explicit.

[1] https://www.typescriptlang.org/docs/handbook/esm-node.html
[2] https://www.typescriptlang.org/tsconfig#node16nodenext-nightly-builds
@slifty slifty force-pushed the dependabot/npm_and_yarn/typescript-5.2.2 branch from b1fd1bf to 87ad78a Compare September 5, 2023 20:21
@slifty
Copy link
Member

slifty commented Sep 5, 2023

Rebased to main -- will merge after CI is all done!

@slifty slifty merged commit fceb24a into main Sep 5, 2023
3 checks passed
@slifty slifty deleted the dependabot/npm_and_yarn/typescript-5.2.2 branch September 5, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants