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.

Sign up for GitHub

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

Jump to bottom

[v21.x] Revert "fs: remove workaround for esm package" #51390

Closed
wants to merge 1 commit into from
Closed

[v21.x] Revert "fs: remove workaround for esm package" #51390

wants to merge 1 commit into from

Conversation

jeremymeng
Copy link

@jeremymeng jeremymeng commented Jan 6, 2024 •
edited
Loading

Fixes: #51081

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. v21.x labels Jan 6, 2024
@jeremymeng
Copy link
Author

per discussion at #51356 (comment), targeting v21.x-staging

H4ad
H4ad previously approved these changes Jan 10, 2024
View reviewed changes
@H4ad H4ad dismissed their stale review January 10, 2024 21:58

Before approving, do we have some docs about reverting changes? Should we actually revert or create a new commit with the change?

H4ad
H4ad approved these changes Jan 10, 2024
View reviewed changes
Copy link
Member

@H4ad H4ad left a comment

Choose a reason for hiding this comment

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

According to reverting commits docs, you only need to include Fixes: #PR in the PR message, can you update that?

@H4ad H4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 10, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56739/

@aduh95 aduh95 changed the title Revert "fs: remove workaround for esm package" [v21.x] Revert "fs: remove workaround for esm package" Jan 10, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jan 10, 2024

you only need to include Fixes: #PR in the PR message, can you update that?

No need to modify the commit message for that, you can edit the PR description and it will be picked up by ncu and added to the commit message upon landing. But not sure if we use the same process for landing PRs on staging branches.

RafaelGSS
RafaelGSS reviewed Jan 10, 2024
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

Why revert it only to v21.x but not main?

@jeremymeng
Copy link
Author

According to reverting commits docs, you only need to include Fixes: #PR in the PR message, can you update that?

Done

@targos
Copy link
Member

targos commented Feb 15, 2024

@RafaelGSS Because the esm package is archived and will never be fixed.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57099/

@targos targos force-pushed the bring-back-workaround-for-esm branch from 1c408b0 to a8d2d56 Compare February 20, 2024 07:56
@targos
Copy link
Member

targos commented Feb 20, 2024

Rebased to include jeremymeng@83f581d

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57192/

@targos targos mentioned this pull request Feb 20, 2024
Closed
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57263/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57264/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57275/

@marco-ippolito
Copy link
Member

marco-ippolito commented Feb 26, 2024 •
edited
Loading

@jeremymeng rebased

@jeremymeng @marco-ippolito
Revert "fs: remove workaround for esm package"
4368d29 
This reverts commit 95b1989.

It appears that the workaround is still needed: issue nodejs#51081
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57458/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 27, 2024 •
edited by marco-ippolito
Loading

🟢 GREEN CI: https://ci.nodejs.org/job/node-test-pull-request/57460/

marco-ippolito pushed a commit that referenced this pull request Feb 27, 2024
@jeremymeng @marco-ippolito
Revert "fs: remove workaround for esm package"
5aacd62 
This reverts commit 95b1989.

It appears that the workaround is still needed: issue #51081

Backport-PR-URL: #51390
PR-URL: #51390
Fixes: #51081
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@H4ad H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2024
@marco-ippolito marco-ippolito added backported-to-v21.x and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 27, 2024
marco-ippolito pushed a commit that referenced this pull request Feb 29, 2024
@jeremymeng @marco-ippolito
Revert "fs: remove workaround for esm package"
f130a33 
This reverts commit 95b1989.

It appears that the workaround is still needed: issue #51081

Backport-PR-URL: #51390
PR-URL: #50907
Fixes: #51081
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@ddribin ddribin mentioned this pull request Feb 29, 2024
Closed
@marco-ippolito
Copy link
Member

Landed in f130a33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@H4ad H4ad H4ad approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@mcollina mcollina Awaiting requested review from mcollina

@aduh95 aduh95 Awaiting requested review from aduh95

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. 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.
7 participants
@jeremymeng @nodejs-github-bot @aduh95 @targos @marco-ippolito @H4ad @RafaelGSS