Uh oh!
There was an error while loading. Please reload this page.
repl: fix referrer for dynamic import - #30609
Conversation
Uh oh!
There was an error while loading. Please reload this page.
d20973d to
83606a7Compare
devsnek
left a comment
There was a problem hiding this comment.
can you put in a comment explaining why the path is being modified?
coreyfarrell
commented
Nov 23, 2019
@devsnek sure but to clarify are you asking for a comment about why |
devsnek
commented
Nov 23, 2019
I am talking about the |
83606a7 to
e6b1d3aCompare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The ESM loader does not accept a directory as the referrer, it requires a path within the directory. Add `/repl` to ensure relative dynamic imports can succeed. Fixes: nodejs#19570
e6b1d3a to
14198ddCompare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
commented
Dec 1, 2019
bcoe
commented
Dec 1, 2019
Windows tests have been quite flaky today, but I don't believe it's related to this PR the most recent failure was |
The ESM loader does not accept a directory as the referrer, it requires a path within the directory. Add `/repl` to ensure relative dynamic imports can succeed. Fixes: #19570 PR-URL: #30609 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>
bcoe
commented
Dec 1, 2019
Landed in 49fb529 |
Congrats on your contribution to the project @coreyfarrell; thanks for helping perfect the ESM functionality. |
The ESM loader does not accept a directory as the referrer, it requires a path within the directory. Add `/repl` to ensure relative dynamic imports can succeed. Fixes: #19570 PR-URL: #30609 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>
coreyfarrell
commented
Dec 2, 2019
A backport of this PR is needed for 12.x. I'm a little unclear from https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md, should I wait 2 weeks before posting a PR against v12.x-staging or do I just post now and it gets blocked for 2 weeks? |
targos
commented
Dec 2, 2019
@coreyfarrell This change doesn't apply cleanly to 12.x because we haven't unflagged ESM there yet. I think we should try to land things in order or it will be difficult to correctly backport everything related to ESM. |
coreyfarrell
commented
Dec 2, 2019
@targos I'm aware it doesn't apply cleanly, I've done the backport in a local branch. Are you saying that ESM will get unflagged in a future 12.x? Is a list being collected of ESM stuff that needs backport to 12.x? |
targos
commented
Dec 2, 2019
There's no list only for ESM stuff, but many PRs related to ESM are semver-minor will have to wait at least for 12.14.0 in January 2020. |
MylesBorins
commented
Jan 12, 2020
hey @coreyfarrell fyi I just backported this to 12.x in 2ccfcd3fab |
The ESM loader does not accept a directory as the referrer, it requires a path within the directory. Add `/repl` to ensure relative dynamic imports can succeed. Fixes: #19570 PR-URL: #30609 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>
The ESM loader does not accept a directory as the referrer, it requires a path within the directory. Add `/repl` to ensure relative dynamic imports can succeed. Fixes: #19570 PR-URL: #30609 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>
The ESM loader does not accept a directory as the referrer, it requires
a path within the directory. Add
/replto ensure relative dynamicimports can succeed.
Fixes: #19570
Checklist
make -j4 test(UNIX)