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

Additions to import.meta #1430

Open
GeoffreyBooth opened this issue Aug 23, 2023 · 34 comments
Open

Additions to import.meta #1430

GeoffreyBooth opened this issue Aug 23, 2023 · 34 comments

Comments

@GeoffreyBooth
Copy link
Member

Opening this issue per today’s TSC meeting. This is a spinoff from nodejs/node#49246 (review):

  • Are additions to import.meta semver-major or semver-minor?
  • Should additions to this shared namespace require WinterCG and/or TSC approval?

An import.meta.node namespace was floated in wintercg/proposal-minimum-common-api#50 (comment), but Bun objected. I think if we want to create a proprietary namespace then we need another WinterCG proposal for the namespace itself.

See also wintercg/proposal-minimum-common-api#54.

@richardlau
Copy link
Member

  • Should additions to this shared namespace require WinterCG and/or TSC approval?

I was under the impression that import.meta was intended to not be a shared namespace -- that implementors could add whatever made sense for their runtime. The issue then becomes if Javascript runtimes start adding useful things to it that are similar how do end users write code using them if they are not common across the runtimes? (e.g. if Bun and Node.js added different import.meta.* APIs to get the filename, how would someone write code that worked both in Bun and Node.js?)

@benjamingr
Copy link
Member

Are additions to import.meta semver-major or semver-minor?

I think semver-minor is fine since unlike new globals I don't expect code to break when we add stuff to import.meta.

Should additions to this shared namespace require WinterCG and/or TSC approval?

I don't think we should require WinterCG approval for anything. I think we should engage with WinterCG in good faith and try to come up with standards and interoperability that work for all of us. I don't think it should block anything certainly not as a policy.

@benjamingr
Copy link
Member

e.g. if Bun and Node.js added different import.meta.* APIs to get the filename, how would someone write code that worked both in Bun and Node.js?

In this case either Bun align (likely), Node.js would align (less but still likely) or someone would write an interop package for people who care about whether or not their code is portable across runtimes.

I do believe standardization is a good idea and asking for feedback is good.

@GeoffreyBooth
Copy link
Member Author

I was under the impression that import.meta was intended to not be a shared namespace

Maybe that’s what TC39 intended, but like a few other things, that’s not what’s happened in practice. We went through years of negotiations with WHATWG to reach consensus on import.meta.resolve, and then through a yearlong PR to move loaders off-thread to match the import.meta.resolve spec defined by the HTML spec (among other reasons). I understand that there are some members of the community who feel that Node should just ignore WHATWG and other standards and do whatever we like, but I think it’s been a resolved policy for many years now that we try to adhere to standards with other runtimes whenever possible, especially whenever we implement APIs that exist in other runtimes.

The unfortunate thing is that import.meta is the only place to put APIs that want to know anything about the current module. For example, we recently added import { register } from 'node:module' and we would love to support register('./file.js') but instead users need to do register('./file.js', import.meta.url) or register(import.meta.resolve('./file.js')) because there’s no way that register can know the URL of the module that calls it; that’s just not possible, except for methods attached to import.meta. So now import.meta becomes this very prime real estate where API designers might want to attach anything that could benefit from knowing about the current or calling module; it’s not just a basket of generic static properties about the module.

In this case either Bun align (likely), Node.js would align (less but still likely) or someone would write an interop package for people who care about whether or not their code is portable across runtimes.

In general I think we want to encourage interoperability of code by default; there are plenty of authors who write packages for Node, or for browsers, that could be compatible across runtimes but because it’s not the path of least resistance, they neglect to do so. The broader user base is better served the more we can make it easy for packages to be written in a cross-compatible way by default.

What would this “interop package” look like? import.meta can only be accessed within the current module; it’s like __filename in that it’s specific to the current module and not shared with other modules. It therefore can’t be polyfilled, other than using module customization hooks to do things like rewrite the source of modules before they’re evaluated (which is a very Node-specific solution). The only reasonable way for users to write compatible code is via things like const absolutePath = import.meta.filename ?? import.meta.path (to cover Node and Bun); there’s no way to write a library to export a helper for this. These annoyances are why I think it’s especially important to adhere to standards around import.meta.

@ljharb
Copy link
Member

ljharb commented Aug 23, 2023

fwiw, a helper would likely look something like wrap(import.meta), and then you'd access from or destructure that object the same as you would import.meta.

@GeoffreyBooth
Copy link
Member Author

My takes:

  • Are additions to import.meta semver-major or semver-minor?

I think semver-minor.

  • Should additions to this shared namespace require WinterCG and/or TSC approval?

I think at least TSC approval, and the TSC should make an effort to get WinterCG approval too. We should also try to add whatever WHATWG adds to import.meta if we can (currently they’ve defined just import.meta.url and import.meta.resolve, both of which we support).

I also think that we should be sparing in granting approvals. There was a suggestion to add import.meta.register here, for import { register } from 'node:module', but to me that feels ridiculous: we shouldn’t be putting things here that are rarely used, just to avoid passing a second argument to a method. I think there’s probably also a performance cost in each thing that we add, as import.meta is an object created anew for every loaded module.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2023

I doubt there's actually a performance cost - {} vs { key: value } should be identically instantaneous, and adding a getter for something that needs computation should be negligible.

@mhdawson
Copy link
Member

I think at least TSC approval, and the TSC should make an effort to get WinterCG approval too.

TSC approval aligns with my thinking at well with the expecation that the TSC should work to use the same values across runtimes when possible.

@jsumners
Copy link

jsumners commented Aug 28, 2023

I was under the impression that import.meta was intended to not be a shared namespace -- that implementors could add whatever made sense for their runtime.

Yep.

I think semver-minor

+1

I don't think we should require WinterCG approval for anything.

+1,000,000

I think we should engage with WinterCG in good faith and try to come up with standards and interoperability that work for all of us. I don't think it should block anything certainly not as a policy.

+1

I understand that there are some members of the community who feel that Node should just ignore WHATWG and other standards and do whatever we like

A couple of things from a non-maintainer perspective:

  • Yes, Node should adhere to standards as much as makes sense. There are going to be standards that just plain do not make sense for Node. In particular, you mention WHATWG. That org is very explicitly designing standards for web browsers and how they should handle HTML / the DOM. It's right there in the FAQ. Node does not run in a web browser. It can be used to run tools for preparing code to run a web browser, but that is not the same thing.

  • It's very frustrating to take the time to submit an improvement only to be met with "well, let's ask a bunch of other people that don't maintain the thing you are trying to improve what they think" and then block waiting on their deliberations. If something makes sense for Node then it should be considered in that light instead of "well, what if people using other things do something else?".

So now import.meta becomes this very prime real estate where API designers might want to attach anything that could benefit from knowing about the current or calling module;

That's literally the spec defined point of it, right?

In general I think we want to encourage interoperability of code by default; there are plenty of authors who write packages for Node, or for browsers, that could be compatible across runtimes but because it’s not the path of least resistance, they neglect to do so. The broader user base is better served the more we can make it easy for packages to be written in a cross-compatible way by default.

I really don't see the point. Non-browser runtimes are by definition not web browsers. It'd be an unmitigated disaster if web browsers all implemented different runtime environments because they are designed to view client agnostic web pages. That's the thing folks like WHATWG are governing. But non-browser runtimes are chosen by developers for the features (API) of those runtimes. I don't think it is unreasonable for those developers to be aware of the ramifications of their choices.

I think at least TSC approval,

Agreed.

and the TSC should make an effort to get WinterCG approval too.

Disagree.

@benjamingr
Copy link
Member

benjamingr commented Sep 6, 2023

From TSC discussion, it seems like we're gaining consensus around:

  • Want feedback from @jasnell .
  • Changes to import.meta should likely be semver-minor.
  • Changes to import.meta should require TSC approval.
  • WinterCG should be consulted with and collaborated with in good faith but is not blocking.
  • Automation as a concern was raised since we currently have automation for semver-major. In order to not miss import.meta changes we should should fix our automation so these changes don't get missed.

@mcollina
Copy link
Member

mcollina commented Sep 6, 2023

@nodejs/tsc @jasnell are there any objections with this plan?


Are additions to import.meta semver-major or semver-minor?

semver-minor

Should additions to this shared namespace require WinterCG and/or TSC approval?

TSC approval, plus WinterCG notified

@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2023

Can we clarify what TSC approval means in this context? Does that mean TSC should vote every time we want to add something to import.meta? Does that apply also to things we want to modify an existing property?

@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2023

IMO requiring TSC approval (whatever that means) is really not necessary, IMO the usual code review process is enough. If there are objections/lack of consensus, the TSC can get involved, but I don't see the point if the addition is already at consensus. AFAICT the current review process works well enough, let's not complicate it further, wdyt?
A better policy would be "TSC and WinterCG should be notified". TSC members (or any collaborator) can object to the PR / add the PR to the TSC agenda if needed.

@GeoffreyBooth
Copy link
Member Author

IMO requiring TSC approval (whatever that means) is really not necessary, IMO the usual code review process is enough.

No, additions here need to be coordinated with other runtimes. That's something the TSC needs to be involved with, if not lead. We can't just have any two collaborators landing whatever they want on here.

@benjamingr
Copy link
Member

I assumed "the TSC is pinged and we operate by lazy consensus and not necessarily a vote".

@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2023

I assumed "the TSC is pinged and we operate by lazy consensus and not necessarily a vote".

If that's the case, +1, but that seems to be better described by "the TSC is notified" rather than "the TSC needs to approve".

That's something the TSC needs to be involved with, if not lead. We can't just have any two collaborators landing whatever they want on here.

The TSC does not own the code, the collaborators do. I don't know what your last sentence is implying, but I don't think I agree with it, the TSC members are not some kind of "better collaborators". Strong -1 on my side to add a vote requirement, it would be an overreach from the TSC, and it would add useless friction when the current process is actually working well enough.

@benjamingr
Copy link
Member

The TSC does not own the code, the collaborators do

+100 to that

@GeoffreyBooth
Copy link
Member Author

We can't just have any two collaborators landing whatever they want on here.

The TSC does not own the code, the collaborators do. I don’t know what your last sentence is implying, but I don’t think I agree with it, the TSC members are not some kind of “better collaborators”.

What I mean is that we shouldn’t have just two approvals be enough here. import.meta is a shared namespace and we need to be more careful with it, including outreach to groups like WinterCG. Making the TSC sign off in some way, like lazy consensus, both ensures that the outreach happens and also that whoever does the outreach can speak for the project as a whole.

We have a policy that any new global is considered a semver-major change, and per https://github.com/nodejs/node/blob/6919d7241657517bfff6d2041748fa9f8e85b76e/doc/contributing/collaborator-guide.md#breaking-changes:

At least two TSC voting members must approve backward-incompatible changes to the main branch.

I don’t think we need to make import.meta additions semver-major, but they should have a similar policy in requiring some kind of TSC sign-off.

@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2023

import.meta is a shared namespace and we need to be more careful with it, including outreach to groups like WinterCG

Isn't that already what's happening with the current process? There have been several proposal of addition for import.meta, and none have landed except import.meta.resolve. What would have changed with the proposal?

@GeoffreyBooth
Copy link
Member Author

Isn’t that already what’s happening with the current process? There have been several proposal of addition for import.meta, and none have landed except import.meta.resolve. What would have changed with the proposal?

I think the point of this issue is to formalize what so far has been an improvised process, so that we do the same for future import.meta proposals. They’ll only get more common, I think, as ESM adoption increases.

Especially with so many sources across the web defining import.meta as host-defined, many contributors might assume that we treat this particular API as much more open for proprietary contributions than we are, at least right now.

@mhdawson
Copy link
Member

I'm in favor of @mcollina's suggestion in #1430 (comment) where TSC approval means the regular lazy consensus process.

@jasnell
Copy link
Member

jasnell commented Sep 13, 2023

Changes to import.meta should likely be semver-minor.
Changes to import.meta should require TSC approval.

I would suggest semver-major as it's a global namespace. That said, see below.

WinterCG should be consulted with and collaborated with in good faith but is not blocking.

The WinterCG has discussed setting up a kind of registry for import.meta terms. I have an outstanding todo to get that started this week. Essentially, WinterCG likely won't standardize the import.meta terms itself but will provide a registry (in the same sense, for instance, that IANA provides a registry for things like http headers, etc) that implementations can use to coordinate use of specific import.meta properties with the key idea being to encourage runtimes not to step on each other when introducing new properties.

With such a registry in place, if runtimes do agree to adhere to the registry and use it as a way of coordinating import.meta properties, then semver-minor is sufficient for adding new properties as the risk of incompatibilities is greatly reduced.

Automation as a concern was raised since we currently have automation for semver-major. In order to not miss import.meta changes we should should fix our automation so these changes don't get missed.

@aduh95
Copy link
Contributor

aduh95 commented Sep 16, 2023

I'm going to say it explicitly: I'm -1 on requesting TSC approval for changes on import.meta. I think this is not a necessary change, and we should resist the TSC granting itself additional power unless it's actually necessary.
I think the TSC should come up with guidelines on how to review a PR proposing changes to import.meta, and should trust the collaborators to follow those guidelines (and those guidelines are not followed for whatever reason, the change can be reverted immediately).

If WinterCG goes forward with their idea described in #1430 (comment), my suggestion for the guidelines would be:


Any change to import.meta cannot land unless:

  • it correspond to one entry in the WinterCG registry.
  • it corresponds to a PR to the WinterCG registry open for at least 7 days (to let other runtimes time to chime in / raise objection if needed, but to not stall the Node.js PR if the other runtimes are irresponsive; 7 days is arbitrary, maybe another value is more suited)
  • it corresponds to a proposal that was rejected by WinterCG, but a TSC vote decided it should still land in Node.js (hopefully that never happens, but it's probably important that the project keeps control over what's landing).

It must also follow the usual process for landing PRs – specifically, that means a feature can be accepted on the WinterCG registry, and still be rejected in Node.js if a collaborator objects to it.


Ideally, the TSC, or at least someone from the project, should be involved in reviewing changes to the WinterCG registry though. IMO any change to that registry would deserve discussion from the TSC, maybe we can have an automation that automatically adds to the TSC agenda any new PR to the registry.
We could add @nodejs/tsc as an "owner" of the file where import.meta properties are set, so the TSC members get notified when a change is suggested, but I'm not sure it's necessary, the above guidelines should be enough.
We should probably NOT make a recommendation on what semverness import.meta changes should be, and instead let the usual process apply (semver-minor when it's an addition, semver-major when it can break someone, semver-patch if it's a fix).

wdyt?

@jasnell
Copy link
Member

jasnell commented Sep 16, 2023

I'm still waiting on more feedback from WinterCG before we can consider this official, but I have the start of the registry here: https://github.com/wintercg/import-meta-registry

@benjamingr
Copy link
Member

Changes to import.meta should likely be semver-minor.
Changes to import.meta should require TSC approval.

I would suggest semver-major as it's a global namespace. That said, see below.

Why? What's the breaking potential - the issue with globals is they can change how JS code executes with regards to scoping - that's not a concern here as far as I'm aware?

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Sep 17, 2023

I’m going to say it explicitly: I’m -1 on requesting TSC approval for changes on import.meta. I think this is not a necessary change, and we should resist the TSC granting itself additional power unless it's actually necessary.
I think the TSC should come up with guidelines on how to review a PR proposing changes to import.meta, and should trust the collaborators to follow those guidelines (and those guidelines are not followed for whatever reason, the change can be reverted immediately).

I disagree with this. Ultimately every PR needs TSC approval, since any block could lead to a TSC vote. Requiring two TSC approvals on an import.meta PR is a way to ensure that the TSC has considered it, and it slows down the process which is necessary in this case because we need to ensure that the specific workflow around import.meta has been followed (opening the proposal on the WinterCG repo, etc.).

It’s not a question of trusting or not trusting the collaborators. It’s simply unreasonable to expect all of them to know about a particular process that’s specific to import.meta. It’s even unreasonable to expect everyone in @nodejs/tsc to necessarily remember that this process needs to be followed, or to react in time to block a PR until it’s done. Requiring two TSC approvals slows a PR down enough to hopefully ensure that someone who remembers what needs to be done has ensured that it happens before the PR lands.

Additions to import.meta should be rare. It’s okay if they’re difficult to land. The more populated that namespace becomes, the more interoperability and portability problems we create.

@aduh95
Copy link
Contributor

aduh95 commented Sep 17, 2023

@GeoffreyBooth do you have any opinions on the guidelines I have drafted?

@GeoffreyBooth
Copy link
Member Author

@GeoffreyBooth do you have any opinions on the guidelines I have drafted?

Yes. The guideline should include that two TSC approvals are required, like we have for globals.

I also think that requiring only that a PR be open for 7 days in the WinterCG repo is way too lax. It should be landed there. The registry is just a place for runtimes to lay claim to names; there’s no reason for other runtimes to ever block a proposal other than “hey, we already use that name.” There’s also no hurry on landing import.meta PRs in Node. We can wait for the WinterCG process to play out.

@benjamingr
Copy link
Member

As mentioned above in #1430 (comment) I object to requiring WinterCG approval for additions, we are placing boundaries on ourselves that none of the other runtime are committing to and we have the biggest obligation to our users out of all of them as the biggest runtime.

Node.js should do what's best for Node.js and the web and should collaborate in good faith with WinterCG but not be subservient to it IMO. Like Node.js can add stuff to import.meta without WHATWG or TC39 approval...

I also agree with TSC approval (through lazy consensus, since we have globality concerns) but haven't heard a strong argument for semver-major. I don't see it as different from adding an argument to a method which isn't major. I am also OK with "regular collaborator process + ping".

@GeoffreyBooth
Copy link
Member Author

I object to requiring WinterCG approval for additions

We can add some kind of condition like “if rejected at WinterCG, or still pending at WinterCG after two months, a lazy consensus or vote of the TSC can decide to proceed without WinterCG approval.” I would be fine with that. But WinterCG only meets once per month so it might take up to two months for a decision to be made by them. (But again, I don’t think it should be easy or quick to land additions to import.meta.)

I agree that regular semver rules should apply, that additions are presumed minor, changes to existing properties are probably major, etc.

@benjamingr
Copy link
Member

@GeoffreyBooth mostly, people in WinterCG today (like James) are great and I trust them but as a standards body it's new and I wouldn't want our process to block on it or wait for it.

What about the following idea:

  • import.meta follows regular contribution rules, either the TSC of a dedicated team is pinged on import.meta changes.
  • Rules follow regular semverness (breaking changes are major, additions are minor).
  • The PR can land with the API being marked as experimental.
  • At that point the team reviews those changes and unless there is a reason to block goes to WinterCG for registration in the registry. If registration fails at WinterCG - the TSC has to decide on the issue.
  • After registration (good) or TSC decision (bad) the API can become stable. Similarly to what we do when implementing TC39 proposals that are not stage 4 yet or shifting web APIs.

The changed requirement is that "import.meta additions cannot move from experimental without either WinterCG registration or TSC approval" which addresses standardisation concerns and "collaborators own the code' concerns (both I think are valid). It also means we explicitly seek WInterCG feedback on import.meta changes which I think is a good thing.

@GeoffreyBooth
Copy link
Member Author

That’s mostly good, I would change a few things:

  • Can land as experimental behind a flag. We’ve had problems in the past where something is marked as experimental in the docs but can be accessed without an experimental flag.
  • Let’s make a @nodejs/import-meta working group and give it a charter it to approve or reject import.meta PRs. No PRs land without this group’s approval, or overruling by the TSC. Having a WG needing to approve would ensure that people who know the process ensure that it’s followed, and therefore we can forgo the two TSC approvals requirement.

@aduh95
Copy link
Contributor

aduh95 commented Sep 17, 2023

The idea of having a separate team dedicated to this SGTM – probably not a WG, and possibly with a broader scope than just import.meta, maybe @nodejs/interoperability or something.

@mhdawson
Copy link
Member

@benjamingr was what you proposed captured in the documentation. Wondering if we have additional actions left or if this issue should be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants