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

[v14.x backport] util: fix module inspection & instanceof check during inspect #37100

Closed
wants to merge 2 commits into from

Conversation

Pezmc
Copy link

@Pezmc Pezmc commented Jan 27, 2021

Backports #36178 from @BridgeAR, tagged as semver‑patch, to 14.x as per comment from @ExE-Boss

14.x is currently impacted by the bug fixed in #36178, which was originally released to 15.x in https://github.com/nodejs/node/releases/tag/v15.5.0

I followed the guide from https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Jan 27, 2021
@Pezmc Pezmc changed the base branch from master to v14.x-staging January 27, 2021 11:57
Pezmc pushed a commit to Pezmc/node that referenced this pull request Jan 27, 2021
Backport-PR-URL: nodejs#37100

Signed-off-by: Ruben Bridgewater <[email protected]>

Fixes: nodejs#36151

PR-URL: nodejs#36178
Fixes: nodejs#35730
Refs: nodejs#35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Pezmc pushed a commit to Pezmc/node that referenced this pull request Jan 27, 2021
Backport-PR-URL: nodejs#37100

Signed-off-by: Ruben Bridgewater <[email protected]>

Fixes: nodejs#35730

PR-URL: nodejs#36178
Fixes: nodejs#36151
Refs: nodejs#35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Pezmc
Copy link
Author

Pezmc commented Jan 27, 2021

I don't have the Jekins permissions to schedule a node-test-pull-request CI job for this PR (37100)

Pezmc is missing the Job/Build permission

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 27, 2021

@BethGriggs
Copy link
Member

Would appreciate a review from someone more familiar with this area (cc @nodejs/modules?)

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM; i'll make similar changes in object-inspect.

Comment on lines +538 to +545
function isInstanceof(object, proto) {
try {
return object instanceof proto;
} catch {
return false;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

why is the try/catch needed? are we worried about someone defining a Symbol.hasInstance method that can throw?

Copy link
Author

Choose a reason for hiding this comment

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

Based on cb9eec9#r527685177, instanceof can throw

Copy link
Member

Choose a reason for hiding this comment

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

ah, didn't realize this was a backport PR. turns out instanceof can indeed throw when the RHS is not a function, or when it's a non-constructible function (like an arrow function or a concise method)

Pezmc pushed a commit to Pezmc/node that referenced this pull request Jan 29, 2021
Backport-PR-URL: nodejs#37100

Signed-off-by: Ruben Bridgewater <[email protected]>

Fixes: nodejs#36151

PR-URL: nodejs#36178
Fixes: nodejs#35730
Refs: nodejs#35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Pezmc pushed a commit to Pezmc/node that referenced this pull request Jan 29, 2021
Backport-PR-URL: nodejs#37100

Signed-off-by: Ruben Bridgewater <[email protected]>

Fixes: nodejs#35730

PR-URL: nodejs#36178
Fixes: nodejs#36151
Refs: nodejs#35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Pezmc
Copy link
Author

Pezmc commented Jan 29, 2021

@BethGriggs I tried rebasing onto the latest nodejs:v14.x-staging to clean this PR after your force push and ended up with 89402af in my diff. Do you know where that came from?

$ git fetch upstream v14.x-staging
From https://github.com/nodejs/node
 * branch                  v14.x-staging -> FETCH_HEAD
$ git checkout --track upstream/v14.x-staging
Updating files: 100% (14760/14760), done.
Branch 'v14.x-staging' set up to track remote branch 'v14.x-staging' from 'upstream'.
Switched to a new branch 'v14.x-staging'
$ git pull
Already up to date.
$ git checkout backport-36178-to-v14.x
Switched to branch 'backport-36178-to-v14.x'
$ git rebase v14.x-staging
Successfully rebased and updated refs/heads/backport-36178-to-v14.x.
$ git checkout -B backport-36178-to-v14.x-test
Switched to a new branch 'backport-36178-to-v14.x-test'
$ git push

Pezmc pushed a commit to Pezmc/node that referenced this pull request Jan 29, 2021
Backport-PR-URL: nodejs#37100

Signed-off-by: Ruben Bridgewater <[email protected]>

Fixes: nodejs#36151

PR-URL: nodejs#36178
Fixes: nodejs#35730
Refs: nodejs#35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Pezmc pushed a commit to Pezmc/node that referenced this pull request Jan 29, 2021
Backport-PR-URL: nodejs#37100

Signed-off-by: Ruben Bridgewater <[email protected]>

Fixes: nodejs#35730

PR-URL: nodejs#36178
Fixes: nodejs#36151
Refs: nodejs#35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs
Copy link
Member

BethGriggs commented Jan 29, 2021

@BethGriggs I tried rebasing onto the latest nodejs:v14.x-staging to clean this PR after your force push and ended up with 89402af in my diff. Do you know where that came from?

Ah yes, that commit was backed out of staging as there was an npm version mismatch (ref: #37107 (comment)). We need to reland the npm update in a new PR.

For this PR, I think you could either rebase against upstream/v14.x-staging and then drop the npm commit. Or I can just cherry-pick your commits from this PR.

The good news is that the delay in fixing the npm update means we can probably squeeze this into v14.15.5 (#37074)

Backport-PR-URL: nodejs#37100

Signed-off-by: Ruben Bridgewater <[email protected]>

Fixes: nodejs#36151

PR-URL: nodejs#36178
Fixes: nodejs#35730
Refs: nodejs#35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Backport-PR-URL: nodejs#37100

Signed-off-by: Ruben Bridgewater <[email protected]>

Fixes: nodejs#35730

PR-URL: nodejs#36178
Fixes: nodejs#36151
Refs: nodejs#35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Pezmc
Copy link
Author

Pezmc commented Jan 29, 2021

Re-rebased and manually dropped that commit, this PR should be good to go @BethGriggs!

@Pezmc Pezmc closed this Jan 29, 2021
@Pezmc Pezmc deleted the backport-36178-to-v14.x branch January 29, 2021 21:54
@Pezmc Pezmc restored the backport-36178-to-v14.x branch January 29, 2021 21:54
@Pezmc
Copy link
Author

Pezmc commented Jan 29, 2021

Accidental close, please ignore!

@Pezmc Pezmc reopened this Jan 29, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 29, 2021

@Pezmc
Copy link
Author

Pezmc commented Feb 1, 2021

@BethGriggs Anything I can do to help get this into v14.15.5?

@BethGriggs
Copy link
Member

@Pezmc, I'll aim to integrate this into the v14.15.5 proposal - i'm still waiting on some other patches so haven't started updating the proposal yet. Nothing more to do on your side 🙂

@BethGriggs BethGriggs added the review wanted PRs that need reviews. label Feb 3, 2021
@BethGriggs
Copy link
Member

Would appreciate a review on this before landing (maybe @nodejs/modules)?

Copy link
Member

@BethGriggs BethGriggs left a comment

Choose a reason for hiding this comment

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

Backport LGTM comparing with #36178

BethGriggs pushed a commit that referenced this pull request Feb 5, 2021
Signed-off-by: Ruben Bridgewater <[email protected]>

Backport-PR-URL: #37100
PR-URL: #36178
Fixes: #35730
Fixes: #36151
Refs: #35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 5, 2021
Signed-off-by: Ruben Bridgewater <[email protected]>

Backport-PR-URL: #37100
PR-URL: #36178
Fixes: #35730
Fixes: #36151
Refs: #35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs
Copy link
Member

BethGriggs commented Feb 5, 2021

Landed in 408c7a65f3...f206505 (v14.x-staging). Thanks @Pezmc

@BethGriggs BethGriggs closed this Feb 5, 2021
BethGriggs pushed a commit that referenced this pull request Feb 5, 2021
Signed-off-by: Ruben Bridgewater <[email protected]>

Backport-PR-URL: #37100
PR-URL: #36178
Fixes: #35730
Fixes: #36151
Refs: #35754
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Pezmc
Copy link
Author

Pezmc commented Feb 15, 2021

@Pezmc Pezmc deleted the backport-36178-to-v14.x branch February 15, 2021 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. review wanted PRs that need reviews. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants