Skip to content

Conversation

@ronag
Copy link
Member

@ronagronag commented Aug 4, 2019

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-botnodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 4, 2019
@ronag
Copy link
MemberAuthor

ronag commented Aug 4, 2019

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

@ronagronag changed the title utils: fix broken esmfs: fix broken esmAug 4, 2019
@ronag
Copy link
MemberAuthor

ronag commented Aug 6, 2019

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

@mcollina
Copy link
Member

cc @jdalton wdyt?

@jdalton
Copy link
Member

LGTM as a workaround until a fix can be published.

@BridgeAR
Copy link
Member

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

Copy link
Member

@BridgeARBridgeAR 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.

@Trott
Copy link
Member

Trott commented Aug 6, 2019

@mcollina Looks good to you as a temporary measure?

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Aug 6, 2019

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

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

Copy link
Member

@mcollinamcollina 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.

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

@trivikrtrivikr 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

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

@Trott
Copy link
Member

Trott commented Aug 6, 2019

@Trott
Copy link
Member

Trott commented Aug 7, 2019

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
Fix to unblock CITGM. See, standard-things/esm#821. PR-URL: nodejs#28957 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@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

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

Belated LGTM and thanks for the ping!

@targos
Copy link
Member

Backport blocked by #21387

BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
Fix to unblock CITGM. See, standard-things/esm#821. PR-URL: #28957 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@ronag@mcollina@jdalton@BridgeAR@Trott@nodejs-github-bot@joyeecheung@targos@trivikr