-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: endorse "worker" and "dom" conditions #40914
Conversation
@nodejs/modules |
It seems best that this page document things the community has actually already adopted, instead of suggesting things for the community to adopt. |
I have no opinions on where this discussion is had or where the list is maintained, but I want to stress that the previous discussion, and the docs, and the lack of alternatives, seems to indicate that should raise discussions here currently. Feel free to point me and future contributors elsewhere for what the proper avenue is. For 'worker' specifically, precedence is set by including angular's 'types' 2 weeks ago, compared to what I deem more popular, webpack supporting it already. If your comment is about 'dom', sure, but I mentioned that, I think it'd be good to not just include 'worker', but also an antonym. |
@wooorm, thank you for the PR. You are on the right track and modifying this list in the core repo is most likely the way to go about doing this today. By making this PR, you seem to have expressed an interest (your response would be appreciated at the link below). |
Thanks for putting some coordination effort together here @wooorm. In general the requirements for listing conditions are quite tough and I think they have to be to avoid ending up with a mess of a conditions buffet. Can definitely see the merit of these, but have to try and think about process too. Here's some feedback - The guidelines for a new condition as listed in the docs state:
So I think the The definition of |
It seems important that something have interoperability - webpack is the most-used bundler, but it's only one. It seems like it'd be good for multiple popular bundlers to agree on the condition name. |
I don’t think so. Similar to how it’s not viable to start each package with To explain the problem in other words:
Agreed. I think the place to discuss and agree on condition names is, well, this issue tracker. I suffixed my original comment with two questions. |
@wooorm I definitely agree there is utility here in the Does The way to move forward would be to get Webpack or other bundlers interested in implementing this first before getting it further defined here. If and when we move this listing to a new repo we could investigate more process around having stages of condition definitions, but right now this is only supposed to be a stable listing. |
I agree on the need to get folks implementing it before listing it, but I also want to stress the need to first discuss it before anyone implements things. For example, webpack implemented |
@wooorm I was not aware Webpack have already implemented |
Yep, they do, see the OP. I found that out, and based on the issue there and the precedence set by The question then is (whether it’s maintained here or not), what this list is:
|
@wooorm this list is basically the next step after bundler implementation - if eg Webpack has implemented but then is wanting to make sure there is clear guidance for other bundlers beyond its own documentation, and to help clarify where coordination between bundlers is needed to move to a place of stability for the meaning of a condition. For example, having it listed here might encourage other bundlers to more easily follow. Worker could possibly fit the definition to be used further, but there is another confusion here and that is if Node.js would be expected to implement this condition for its worker threads? |
I think the first is true: /cc @alexander-akait @sokra from webpack |
@wooorm have other bundlers also added support for it, or been asked to do so? |
@ljharb No. |
@wooorm it does not feel appropriate to me for node to document anything before it gains widespread community adoption (multiple implementations) - that's prescriptive, and node's community docs should be descriptive. |
I understand that. I’m open to it (when maintained here). Though from the rest of the conversation it sounds to me like other people see this list differently, and importantly it’s documented differently: “endorsed” and “user conditions”. Maybe good to try and change the recommendation in the docs? Either to make it clear that you don’t want things like |
@wooorm the conditions requirements clearly state:
The other requirement I'm calling out here is:
I have specifically asked if Node.js should implement the |
This patch is the “user conditions” list, endorsed but not supported by Node.js. I believe you’re looking for the answer “no”.
I expressed that from the start. The reason I think it’s wise to discuss things beforehand is exactly the problems with |
Why would Node.js workers not be included? Would Deno be expected to support the
It's not about what I find acceptable - it's about meeting the requirements for listing a condition which clearly state there must be existing prior implementation. If you want to change to do something that goes against the current guidance, then a new issue should be created to change the current guidance first. |
|
@wooorm the The documentation is very clear on this:
I'm specifically calling out with the
To fulfill that requirement means making the answer to the question "does Node.js or Deno or another JS platform implement the worker condition" absolutely clear in the condition definition. |
Can you provide proof that TS supports the
The description I provided seems crystal clear to me. Does Node.js/Deno/whatever implement “web workers (scripts that run in browser background threads)”? Are these platforms browsers? Do they support web workers? Could you perhaps suggest an edit to that line with actionable feedback on what you find vague and how that can be solved? I’m stuck in a catch-22 here.
1 is being ignored, 2 is being rejecting on the grounds that it wasn’t discussed first, and 3 because it isn’t implemented 🤷♂️ I care about the problem: Some bundlers are picking the 10kb extra in browsers. Other bundlers are sending |
@wooorm I'm trying to understand what you'd like to achieve here, it's just important to ensure it can fit into a clear conditions definition for all environments. These definitions can never be removed once added - their meanings are permanent. So definition friction and discussion is critical. I'm not just trying to give you a hard time, honest. I agree with you that {
"exports": {
"dom": "./lib-dom.js",
"default": "./lib-nodom.js"
}
} seems like it will be much more widely supportable than even having to use the My concerns around specifying Then in terms of building workflows around a Perhaps create a new PR for the |
Glad to hear! Indeed. The list of exceptions is becoming long (I need to add
The reason I added
Nothing. I consider this PR “pending implementation” and a place to discuss how it should be called before implementations. But I don’t think, just like with Perhaps a way to unblock this, is to add some separate lists.
Maybe this makes more sense for for when this list is moved elsewhere. But that might just take a lot of time? |
No they are not and likely won't be in current sense of the term. That said, I definitely agree that |
Cloudflare are now using the I believe this extends the definition of |
Reading that issue though, it seems that the |
Your original reasoning seems sound to me honestly. 'Worker' defining an environment without DOM - ie. worker like (cloudflare worker, web worker, etc). And 'dom' as the catch all antonym. Both seem useful and don't conflict. |
This adds two new conditions to the endorsed conditions list in the Node.js documentation.
Their goal is to be more specific that the
browser
condition (and its predecessor, the browser field), around DOM APIs, specifically for web workers and non-web-workers.The problem this solves is that most bundlers have a switch that is either
browser
ornode
, but this switch is not specific enough for web workers (or their alternative, “normal” web scripts/modules), which don’t have access to most importantly the DOM, but don’t have access to Node modules such aspath
orcrypto
or you name it. Folks (or tooling), typically pick thebrowser
condition for web workers, but I as a package maintainer usebrowser
to choose code that uses DOM APIs *(which is explicitly mentioned as a valid use case by thebrowser
field spec).A practical example is: https://github.com/wooorm/parse-entities/blob/6d7cef95baa04c3430f929cb017b541f54b9e863/package.json#L28-L40, although there isn’t a
worker
field, so that isn’t supported there. But what it does is: Use the full package by default, switch to the DOM version (which is very small because it uses DOM APIs) in browsers, and switch back to the default in places that don’t have a DOM (react-native, which is currently mentioned on the latest website but not in this file I changed).This is why I propose a
worker
condition, which is already supported in webpack. But also it’s inverse (dom
), because I think similar to dev/prod, it makes sense to have both.Currently, webpack supports
worker
, and esbuild and rollup don’t (but then again webpack does workers out of the box and the rest allows arbitrary conditions).dom
is a name I made up myself.Here are links to the relevant code in what I believe are the primary bundlers for the web and where they handle conditions:
worker
)Qs:
worker
has support in webpack, but on the other hand, it seems similar to thereact-native
field that is currently on the website but no longer in the source files, in that it means “no dom”. Open to alternativesdom
has no existing support, but i think it makes sense to provide an antonym toworker
, because otherwise I as a package maintainer have to list all the platforms that have no DOM (worker, react-native, etc)/cc @guybedford @defunctzombie @alexander-akait
Related-to: GH-40708 (in that this is my first PR to Node and I modelled it after a similar PR)
Can someone ping @nodejs/modules I guess?