| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I pushed a commit to fix some lint issues, but this still won't compile for me, I believe because @ ends up in variable names inside the generated node_javascript.cc file. Looks like @devsnek has fixed this. Go team! |
Sorry, something went wrong.
|
@nodejs/npm @nodejs/security @nodejs/modules ... we really need to make sure we are coordinating on this to make sure we do not accidentally introduce any security issues with older versions of Node.js that do not understand the @nodejs/ prefix. |
Sorry, something went wrong.
|
@jasnell I believe we can (and should) backport the resolver to all current LTS versions of the platform. If we consider it a security patch we can do so as a patch |
Sorry, something went wrong.
|
@jasnell as long as node owned the nodejs account, I’m not aware of any possible security issues, backported or not (I’m always in favor of maximal backporting, ofc) |
Sorry, something went wrong.
While I'm +1 on the idea of putting new modules under a namespace, I don't think the PR enabling this should alias existing modules.
|
Sorry, something went wrong.
|
I don’t think it’s a good idea to be opportunistic here and try to pile on “improvements” on top of namespacing. |
Sorry, something went wrong.
|
I know not everyone will agree. That's why I think it should be a separate discussion that shouldn't block new modules to be under a namespace. |
Sorry, something went wrong.
|
I do not think this is the correct approach, and a full implementation will require doing this via the resolver. Every file that we add increase the startup time, and we are currently trying to reduce it. |
Sorry, something went wrong.
|
I agree with @mcollina ... This is something that can be done within the resolver. Adding this kind of indirection is not the right approach. |
Sorry, something went wrong.
|
That’s totally fine; the implementation of that is equally straightforward. @mcollina do i need to put up a new PR that contains a string to string mapping, or would we be able to defer debating the implementation details until after the underlying direction has been decided? |
Sorry, something went wrong.
|
We can debate on the implementation details after the @nodejs/tsc agrees with the approach. |
Sorry, something went wrong.
|
Update: TSC has provisionally agreed on the approach; I'll update this PR with an actually workable implementation (this was originally just a prototype) |
Sorry, something went wrong.
|
What's the progress on this? |
Sorry, something went wrong.
|
@jasnell i have to update the PR based on #21551 (comment); conferences, travel, and life have interfered for the last month or two. I'm still planning to get on it soon :-) |
Sorry, something went wrong.
|
@ljharb Are you able to rebase this and we can pick it up again? |
Sorry, something went wrong.
|
@Trott sure, i'll try to do that now. (edit: i talked to myles; he'll replace my commits with his own) |
Sorry, something went wrong.
|
I had started working on something after seeing @Trott's ping and with @ljharb's blessing I've force pushed an alternative implementation of this that works via the resolver. I'm pretty sure that this can be improved by moving some of the logic to C++, but wanted to get some feedback before pushing much further. The majority of the implementation is in 4fdc880 48779a5 updates tests and benchmarks... so I've made it a separate commit to make it easier to review without the noise. My initial implementation was with node:. Some rational for that approach
As the PR here had originally been made with @nodejs/ I patched that behavior in with 48779a5 One thing to note, the implementation currently throws if you attempt to load anything from the @nodejs/ namespace that doesn't already exist there. Without removing this we would be unable to polyfill modules, this may be a feature. TL;DR of what I think we need to reach consensus on (which may require separate issues)
/cc @nodejs/tsc @nodejs/modules |
Sorry, something went wrong.
|
I would like to avoid, if at all possible, a situation where we migrate twice:
This leaves us in a situation where:
(Which, to me, seems like unnecessary technical and UX debt.) |
Sorry, something went wrong.
|
Because we cross-posted, just want to say I share @Fishrock123's concerns, he decribes the worst-case that we don't want, but it sounds like the URI-like prefixes (like nodejs:) is going to be how these things are done, but in the browser and in servers. |
Sorry, something went wrong.
|
Shipping nodejs: for all current core modules at this point seems completely sensible to me. Will someone update this PR or create a new one for that? |
Sorry, something went wrong.
|
I'd like to propose a different look at this: Schemes versus Namespaces That said, nodejs: (or [whatever]:) working as described above being the spec'd resolution behaviour for that scheme, and that possibly extending to cover resolutions of nodejs:[whitelisted-vendor-specifiers-only] against node_modules/@nodejs/… somehow (ie #28062) with a lot more details to be figured out obviously. |
Sorry, something went wrong.
|
This was discussed at last TSC meeting, does it still need to be on the upcoming meeting's agenda? |
Sorry, something went wrong.
|
removed from the agenda. Conversations need to be had in this or another thread. |
Sorry, something went wrong.
|
Was there some sort of outcome from the tsc meeting? |
Sorry, something went wrong.
|
@devsnek the outcome was the move forward with namespaces... in a new PR. Folks did not have strong opinions but in general were erring on the side of nodejs: rather than @nodejs/. I've followed up with @ljharb to see if he is ok with us closing this and opening a new PR. |
Sorry, something went wrong.
|
Do we have a feeling for how close we are on a call here? We're starting to use node: internally for multiple purposes (ESM resolver, open PR for policies #28767). Some people have mentioned nodejs: as the official scheme. Unfortunately @nodejs/ isn't really an alternative since it doesn't create valid URLs. So at least internally (and it's likely to leak) we'd have to still use some scheme in addition. Unless we make another call, I assume node: is the thing that exists and would stick around..? |
Sorry, something went wrong.
|
node: is an implementation detail of the esm loader, for the purposes of public schemes i think we should discount its existence. |
Sorry, something went wrong.
It's an implementation detail until it stops being one. The longer we delay moving that implementation detail to the official solution, the higher the risk that we unflag with it, it does leak in some use cases, and we're stuck with it unless we break the ecosystem. |
Sorry, something went wrong.
I needed some URL mapping for policies, which are not integrated with ESM so it also exists in policies under the PR @jkrems linked. So it exists in at least 2 places currently. For policies it is meant to be publicly used. |
Sorry, something went wrong.
|
I recall first noticing it in electron and nwjs — in dev tools — not that I am saying it should not, just how it ends up there can be relevant to keep in mind. |
Sorry, something went wrong.
|
What should we do with this? Close it? Push it forward? Put it on the TSC agenda to see if it can get traction with someone else? |
Sorry, something went wrong.
|
I'm still planning on picking this up. The built in modules proposal is
still being worked on at tc39 and there is hope for stage advancement at
next week's meeting.
If it lands at TC39 I will likely explore and experimental implementation
of it for core
…On Thu, Nov 28, 2019, 12:05 AM Rich Trott ***@***.***> wrote:
What should we do with this? Close it? Push it forward? Put it on the TSC
agenda to see if it can get traction with someone else?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21551?email_source=notifications&email_token=AADZYVZMOMJWTB5ZB6WSMRLQV5GTHA5CNFSM4FHDCYU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFLON6A#issuecomment-559343352>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZYV5WTQTK2TWBKSY23QDQV5GTHANCNFSM4FHDCYUQ>
.
|
Sorry, something went wrong.
|
It’s not on the agenda for next week’s meeting and the stage advancement deadline has passed. |
Sorry, something went wrong.
|
I have mispoken then, didn't check agenda, was under impression it was
going to be on agenda
Apologies
…On Thu, Nov 28, 2019, 11:50 AM Jordan Harband ***@***.***> wrote:
It’s not on the agenda for next week’s meeting and the stage advancement
deadline has passed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21551?email_source=notifications&email_token=AADZYV7DT5XJ3GKQ6UK6D3DQV7ZGLA5CNFSM4FHDCYU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFNDB5Y#issuecomment-559558903>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZYV2ZCLUX3T2W5JK7LG3QV7ZGLANCNFSM4FHDCYUQ>
.
|
Sorry, something went wrong.
|
this pr isn't about js std anyway right? |
Sorry, something went wrong.
|
This PR isn't, but we have discussed which semantics to use for the
namespace and similar behavior to what TC39 uses for built in modules would
be desirable IMHO
…On Thu, Nov 28, 2019, 12:51 PM Gus Caplan ***@***.***> wrote:
this pr isn't about js std anyway right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21551?email_source=notifications&email_token=AADZYV6I2KPNF3GSQMKXL7TQWAAJDA5CNFSM4FHDCYU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFNGOPI#issuecomment-559572797>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZYV43LBPKTPLRN4SGDTTQWAAJDANCNFSM4FHDCYUQ>
.
|
Sorry, something went wrong.
|
This has not been updated in quite a while, has not seen further progress, is out of date. Closing but can reopen if it is picked back up again and updated |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Per nodejs/TSC#389 (comment)
Do not merge this - this is solely a demonstration of how a namespaced module could exist.
Existing core modules would be aliased, either like this, using symlinks, or in the CJS/ESM loaders.
New modules would live inside lib/@nodejs, just like current ones live inside lib/.
Checklist