Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore(NODE-6488): specify target=es2021 for ts compilation tests #4303

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Oct 31, 2024

Description

What is changing?

Set target to es2021 to avoid errors related to supporting JS syntax below our lowest supported node version.

Is there new documentation needed for these changes?

Yes, added to the readme

What is the motivation for this change?

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@baileympearson baileympearson changed the title fix lint chore(NODE-6488): specify target=es2021 for ts compilation tests Oct 31, 2024
@baileympearson baileympearson marked this pull request as ready for review October 31, 2024 18:13
@nbbeeken nbbeeken self-assigned this Nov 7, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Nov 7, 2024
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

I went to leave a bit of breadcrumb in the PR description and when I got to the "documentation?" question is made me think that maybe we should update our "Typescript Version" section with this change. We say we support 4.4 but now we're specifically saying we support 4.4 with a target later than es2021 right?

If we introduced a private field like the node types did, our tests would pass, but anyone not using this target or later would no longer be able to compile with our types, just as we weren't able to with the node type changes, right?

@baileympearson
Copy link
Contributor Author

I went to leave a bit of breadcrumb in the PR description and when I got to the "documentation?" question is made me think that maybe we should update our "Typescript Version" section with this change. We say we support 4.4 but now we're specifically saying we support 4.4 with a target later than es2021 right?

If we introduced a private field like the node types did, our tests would pass, but anyone not using this target or later would no longer be able to compile with our types, just as we weren't able to with the node type changes, right?

Yes, but I think that is fine because that aligns with our runtime behavior. Node16 is the first Node version that supports 100% of es2021. We are free to use 100% of the syntax in es2021 in our codebase (including private fields).

Happy to reconsider though.

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

You sound confident enough for me! let's just capture the new expectations in the Readme in our 'TS Version' section.

I assume this is a pattern we will follow moving forward right? so when we bump min node version we can also bump the required target to compile?

@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Nov 13, 2024
@nbbeeken nbbeeken merged commit 1320ad8 into main Nov 14, 2024
27 of 29 checks passed
@nbbeeken nbbeeken deleted the fix-lint branch November 14, 2024 18:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants