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

[v12.x backport] CJS exports detection #35405

Closed

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Sep 29, 2020

This backports the CJS exports detection from #35249 and the error message adjustments from #35426 for 12.x.

This PR is based to the modules backports PR at #35385 so should land after that.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 29, 2020

Review requested:

  • @nodejs/community-committee
  • @nodejs/modules
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Sep 29, 2020
@guybedford guybedford force-pushed the 12-cjs-export-detection branch 3 times, most recently from 3627a5a to 0e3380a Compare October 1, 2020 03:56
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Approving that these PRs should be backported; haven’t specifically re-reviewed the code since the original PRs.

@codebytere
Copy link
Member

@guybedford can you rebase this?

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

rslgtm

PR-URL: nodejs#35249
Reviewed-By: Mary Marchini <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs#35426
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@guybedford
Copy link
Contributor Author

@codebytere sure, thanks for looking into this, I've rebased the commits.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@codebytere
Copy link
Member

Landed in 2f3ffc0...5357a05

@codebytere codebytere closed this Oct 4, 2020
codebytere pushed a commit that referenced this pull request Oct 4, 2020
PR-URL: #35249
Backport-PR-URL: #35405
Reviewed-By: Mary Marchini <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 4, 2020
PR-URL: #35426
Backport-PR-URL: #35405
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins
Copy link
Contributor

@codebytere we need to also include #35483 and preferably #35501 as well. When is the 12.x release planned for? We may want to bump some of the modules changes that only came out last week if we can have them bake a bit longer.

@codebytere
Copy link
Member

codebytere commented Oct 5, 2020

_@MylesBorins it's slated for tues 10/6 - what are your thoughts for which to bump? we could either

  1. remove the ones i just backported
  2. bump the release a week

@MylesBorins
Copy link
Contributor

There are a bunch of modules backports that were part of this and the prior PR that have not been on Current for two weeks (they went out last week). Even if we bump a week some of the fixes haven't gone out yet. Are we still planning one more minor before maintenance?

@aduh95
Copy link
Contributor

aduh95 commented Oct 23, 2020

Closing in favor of #35757

@aduh95 aduh95 closed this Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants