GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [v20.x backport] backport: esm: require braces for modules code by GeoffreyBooth · Pull Request #50268 · nodejs/node

Conversation

@GeoffreyBooth

@GeoffreyBooth GeoffreyBooth commented Oct 19, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This backports:

  • esm: require braces for modules code #49657

Resolves #49657 (comment).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@GeoffreyBooth
GeoffreyBooth requested a review from ruyadorno October 19, 2023 04:12
@nodejs-github-bot

nodejs-github-bot commented Oct 19, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. v20.x labels Oct 19, 2023
@GeoffreyBooth
esm: require braces for modules code
83e582a 
PR-URL: nodejs#49657
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@GeoffreyBooth
GeoffreyBooth force-pushed the backport-49657-to-20 branch from fc934ad to 83e582a Compare October 19, 2023 04:18
@GeoffreyBooth GeoffreyBooth changed the title src: throw DataCloneError on transfering untransferable objects; esm: require braces for modules code backport: esm: require braces for modules code Oct 19, 2023
@GeoffreyBooth GeoffreyBooth removed the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Oct 19, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 19, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54990/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth GeoffreyBooth changed the title backport: esm: require braces for modules code [v20.x backport] backport: esm: require braces for modules code Oct 19, 2023
targos
targos approved these changes Oct 19, 2023
View reviewed changes
JakobJingleheimer
JakobJingleheimer approved these changes Oct 19, 2023
View reviewed changes
aduh95
aduh95 reviewed Oct 19, 2023
View reviewed changes
Comment thread .eslintrc.js
Comment on lines +114 to +121
{
files: [
'lib/internal/modules/**/*.js',
],
rules: {
'curly': 'error',
},
},

@aduh95 aduh95 Oct 19, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you remove this to avoid getting error when backporting commits that landed before this rule was introduced?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@targos targos Oct 19, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If there are commits to backport from before this rule, I highly recommend to backport them first.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Oct 19, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't know if they actually are any, regardless having less strict lint rules on LTS lines seems like a good idea to me, but it's just a nit

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@GeoffreyBooth GeoffreyBooth Oct 19, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

So I looked into this, and the only two commits from before this rule are:

  • src: throw DataCloneError on transfering untransferable objects #47604, which is semver-major so we won’t backport it.
  • esm: remove globalPreload hook (superseded by initialize) #49144, which @aduh95 requested not be backported. It also wouldn’t run afoul of this rule regardless, because it’s almost all deletions and the few additions include the newly required braces.

So I think the rule is fine to backport, and I’d rather not create unnecessary divergences from main. This rule is also very easy to abide by, and the linter can fix for it automatically.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
This was referenced Oct 20, 2023
CI Reliability 2023-10-20 nodejs/reliability#692
Open
CI Reliability 2023-10-21 nodejs/reliability#693
Open
@targos

targos commented Nov 11, 2023

Copy link
Copy Markdown
Member

Landed in 0103979
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request Nov 11, 2023
@GeoffreyBooth @targos
esm: require braces for modules code
0103979 
PR-URL: #49657
Backport-PR-URL: #50268
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@targos targos closed this Nov 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments
@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes
@ruyadorno ruyadorno Awaiting requested review from ruyadorno
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@GeoffreyBooth @nodejs-github-bot @targos @JakobJingleheimer @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.