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

fs: fix broken esm #28957

Closed
wants to merge 1 commit into from
Closed

fs: fix broken esm #28957

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 4, 2019 •
edited
Loading

Tries to fix blocked CITGM. See, standard-things/esm#821.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 4, 2019
@ronag
Copy link
Member Author

ronag commented Aug 4, 2019

@Trott: can I get a CITGM on this one?

@ronag ronag changed the title utils: fix broken esm fs: fix broken esm Aug 4, 2019
@ronag
Copy link
Member Author

ronag commented Aug 6, 2019 •
edited
Loading

@benjamingr can you start a CITGM on this one? ping @Trott

@mcollina
Copy link
Member

mcollina commented Aug 6, 2019

cc @jdalton wdyt?

@jdalton
Copy link
Member

jdalton commented Aug 6, 2019

LGTM as a workaround until a fix can be published.

@BridgeAR
Copy link
Member

BridgeAR commented Aug 6, 2019

@jdalton did your recent change in esm not fix the issue?

BridgeAR
BridgeAR approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM as intermediate fix. This should definitely be removed as soon as possible though.

lib/internal/fs/utils.js Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Aug 6, 2019

@mcollina Looks good to you as a temporary measure?

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Aug 6, 2019 •
edited
Loading

@Trott: can I get a CITGM on this one?

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1923/

mcollina
mcollina approved these changes Aug 6, 2019
View reviewed changes
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 as long as CITGM passes.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 6, 2019
trivikr
trivikr approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

LGTM, one minor nit

lib/internal/fs/utils.js
@@ -345,6 +345,10 @@ function Stats(dev, mode, nlink, uid, gid, rdev, blksize,
Object.setPrototypeOf(Stats.prototype, StatsBase.prototype);
Object.setPrototypeOf(Stats, StatsBase);

// HACK: Workaround for https://github.com/standard-things/esm/issues/821.
Copy link
Member

Choose a reason for hiding this comment

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

nit: period becomes part of link

Suggested change
// HACK: Workaround for https://github.com/standard-things/esm/issues/821.
// HACK: Workaround for https://github.com/standard-things/esm/issues/821

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Aug 6, 2019

CITGM this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1923/

CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1925/

@Trott
Copy link
Member

Trott commented Aug 7, 2019 •
edited
Loading

CITGM master: 84 failures

CITM this PR: 29 failures

CI is green

This has two TSC approvals (necessary for semver-major)

Landing.

Trott pushed a commit to Trott/io.js that referenced this pull request Aug 7, 2019
@ronag @Trott
fs: fix (temporary) for esm package
320402c 
Fix to unblock CITGM. See,
standard-things/esm#821.

PR-URL: nodejs#28957
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott
Copy link
Member

Trott commented Aug 7, 2019

(@joyeecheung If you have an issue with this, we can revert it, of course.)

@Trott
Copy link
Member

Trott commented Aug 7, 2019

Landed in 320402c

@Trott Trott closed this Aug 7, 2019
@joyeecheung
Copy link
Member

Belated LGTM and thanks for the ping!

@targos
Copy link
Member

targos commented Aug 19, 2019

Backport blocked by #21387

BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
@ronag @BridgeAR
fs: fix (temporary) for esm package
6ff803d 
Fix to unblock CITGM. See,
standard-things/esm#821.

PR-URL: #28957
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
Merged
@Trott Trott mentioned this pull request Oct 8, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@Trott Trott Trott approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ronag @mcollina @jdalton @BridgeAR @Trott @nodejs-github-bot @joyeecheung @targos @trivikr