★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24183Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

test: esm loader unknown builtin module #24183

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@jasnell jasnell

@devsnek devsnek

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
code-and-learn test
Projects
None yet
Milestone
No milestone
8 participants
@franher @mhdawson @devsnek @richardlau @gireeshpunathil @jasnell @BridgeAR @nodejs-github-bot
@franher
Copy link
Member

franher commented Nov 6, 2018 •
edited

Description

New test added as part of code-learn session. It should not land until #24175 is resolved.

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

@nodejs-github-bot nodejs-github-bot added the test label Nov 6, 2018

@franher franher referenced this pull request Nov 6, 2018

Closed

esm loader error not catchable #24175

@mhdawson

This comment has been minimized.

Sign in to view
Copy link
Member

mhdawson commented Nov 6, 2018

The test looks good to me, and we checked with @BridgeAR who believes it is a problem that he has seen and believes it is a bug in Node.js

@mhdawson mhdawson added the code-and-learn label Nov 6, 2018

@franher franher force-pushed the franher:unknonw-builtin-module-error branch from 15caf66 to b9c4915 Nov 6, 2018

@franher

This comment has been minimized.

Sign in to view
Copy link
Member Author

franher commented Nov 6, 2018

Tests are now passing after fixing the resolve hook following @devsnek advice (#24175 (comment)).
@BridgeAR

BridgeAR approved these changes Nov 6, 2018
View changes

Copy link
Member

BridgeAR left a comment

Thanks a lot for the awesome work!
@franher

This comment has been minimized.

Sign in to view
Copy link
Member Author

franher commented Nov 8, 2018

@BridgeAR hello, do you know why the CI is failing at the linter phase?

It is not indicating a linter error but a CI crash, imho.

Thank you for your support.
@devsnek

This comment has been minimized.

Sign in to view
Copy link
Member

devsnek commented Nov 8, 2018 •
edited

seems like the version of node on our ci doesn't support catch without a binding wrt

node/.eslintrc.js

Line 20 in 1f6c4ba

} catch {
cc @nodejs/build
@richardlau

This comment has been minimized.

Sign in to view
Copy link
Member

richardlau commented Nov 8, 2018

seems like the version of node on our ci doesn't support catch without a binding wrt

node/.eslintrc.js

Line 20 in 1f6c4ba
} catch {
cc @nodejs/build

This was fixed in #24198. I've restarted the Travis CI for this PR.
@jasnell

jasnell approved these changes Nov 10, 2018
View changes

@devsnek

devsnek approved these changes Nov 10, 2018
View changes

@franher

This comment has been minimized.

Sign in to view
Copy link
Member Author

franher commented Nov 12, 2018

Looking forward to see this PR landed :)
@gireeshpunathil

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Nov 12, 2018 •
edited

@franher - sure, let me run a full CI and then we should be good to go.
CI: https://ci.nodejs.org/job/node-test-pull-request/18555/
@gireeshpunathil

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Nov 12, 2018

landed as 0229e37 , thanks!

@gireeshpunathil gireeshpunathil closed this Nov 12, 2018

gireeshpunathil added a commit that referenced this pull request Nov 12, 2018

@franher @gireeshpunathil
test: esm loader unknown builtin module 
PR-URL: #24183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Loading status checks…
0229e37

BridgeAR added a commit that referenced this pull request Nov 14, 2018

@franher @BridgeAR
test: esm loader unknown builtin module 
PR-URL: #24183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Verified
This commit was signed with a verified signature.
@BridgeAR BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
cb4c2dd

@BridgeAR BridgeAR referenced this pull request Nov 14, 2018

Merged

Release proposal: v11.2 #24350

kiyomizumia added a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018

@franher @kiyomizumia
test: esm loader unknown builtin module 
PR-URL: nodejs#24183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
cdcafc1

This was referenced Nov 15, 2018

Closed
Closed
Closed
Closed
Closed
Closed

codebytere added a commit that referenced this pull request Jan 12, 2019

@franher @codebytere
test: esm loader unknown builtin module 
PR-URL: #24183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Verified
This commit was signed with a verified signature.
@codebytere codebytere Shelley Vohr
GPG key ID: F13993A75599653C Learn about signing commits
817d871

@codebytere codebytere referenced this pull request Jan 15, 2019

Merged

v10.15.1 proposal #25346

@beevelop beevelop referenced this pull request Jan 29, 2019

Closed

2019-01-29 Version 10.15.1 'Dubnium' (LTS) @codebytere #280

This was referenced Jan 29, 2019

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.