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

chore!: bump typescript to 5.7.3 #5137

Closed
wants to merge 4 commits into from

Conversation

cardoso
Copy link
Contributor

@cardoso cardoso commented Jan 17, 2025

Details

Since bumping typescript may cause type declarations to change even in minor versions, this PR can only be merged with the next lwc major release.

In this repo, I couldn't observe any new errors. It would be interesting to see if running this in CI would surface any downstream breakages that can be listed here and perhaps avoided.

For reference, here are the behavioral changes in versions since current (5.4): 5.5, 5.6, 5.7.

If typescript 5.8 is released before this is merged, it may be worth updating this PR to use that version. It fixes a possible regression, although unlikely to affect anything here: microsoft/TypeScript#60506 (nevermind, this fix was backported to 5.7)

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.
  • 💔 Yes, it does introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.
  • 🔬 Yes, it does include an observable change.

GUS work item

@nolanlawson
Copy link
Contributor

/nucleus test

@nolanlawson
Copy link
Contributor

/nucleus test

Copy link
Contributor Author

@cardoso cardoso left a comment

Choose a reason for hiding this comment

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

Might need to play some config tetris with eslint / eslint-typescript / typescript , but the CI error below should be fixed now.

$ eslint . --cache

/home/runner/work/lwc/lwc/packages/@lwc/module-resolver/scripts/test/matchers/to-throw-error-with-code.ts
  0:0  error  Parsing error: /home/runner/work/lwc/lwc/packages/@lwc/module-resolver/scripts/test/matchers/to-throw-error-with-code.ts was included by allowDefaultProject but also was found in the project service. Consider removing it from allowDefaultProject

/home/runner/work/lwc/lwc/packages/@lwc/module-resolver/scripts/test/matchers/to-throw-error-with-type.ts
  0:0  error  Parsing error: /home/runner/work/lwc/lwc/packages/@lwc/module-resolver/scripts/test/matchers/to-throw-error-with-type.ts was included by allowDefaultProject but also was found in the project service. Consider removing it from allowDefaultProject

/home/runner/work/lwc/lwc/packages/@lwc/module-resolver/scripts/test/setup-test.ts
  0:0  error  Parsing error: /home/runner/work/lwc/lwc/packages/@lwc/module-resolver/scripts/test/setup-test.ts was included by allowDefaultProject but also was found in the project service. Consider removing it from allowDefaultProject

Comment on lines -52 to -57
allowDefaultProject: [
// I'm not sure why these files aren't picked up... :\
'packages/@lwc/module-resolver/scripts/test/matchers/to-throw-error-with-code.ts',
'packages/@lwc/module-resolver/scripts/test/matchers/to-throw-error-with-type.ts',
'packages/@lwc/module-resolver/scripts/test/setup-test.ts',
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now they're picked up and we're also not sure why! 😄

Copy link
Contributor Author

@cardoso cardoso Jan 17, 2025

Choose a reason for hiding this comment

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

Probably this: https://devblogs.microsoft.com/typescript/announcing-typescript-5-7/#searching-ancestor-configuration-files-for-project-ownership

It only affects whatever interfaces with tsserverlibrary's ProjectService (eg: typescript-eslint). Doesn't affect output.

Comment on lines 49 to 52

parserOptions: {
projectService: true,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Third time's a charm.

@nolanlawson
Copy link
Contributor

/nucleus test

@wjhsf
Copy link
Collaborator

wjhsf commented Jan 17, 2025

One of the nucleus failures (LWR) seems to be a type error.

src/compiler.ts(115,62): error TS2345: Argument of type '{ namespace: string | undefined; ... }' is not assignable to parameter of type 'TransformOptions'.
  Types of property 'namespace' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

Seems plausibly related to this change, but also maybe not? I can investigate next week.

@cardoso
Copy link
Contributor Author

cardoso commented Jan 17, 2025

Assuming the nucleus test actually caught downstream breakages, it would be nice to have a summary here. Some might be fixed by more explicit annotation, the others documented here for inclusion in the changelog.

@cardoso
Copy link
Contributor Author

cardoso commented Jan 22, 2025

@wjhsf the nucleus failure seems related to what I describe in #5158 rather than this PR. Would it be better to make namespace and name optional again until next major release or would it be better to fix downstream?

@cardoso
Copy link
Contributor Author

cardoso commented Jan 25, 2025

Superseded by #5169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants