-
Notifications
You must be signed in to change notification settings - Fork 43
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I object super strongly to the tone of this being "we've fixed the mjs thing you were all complaining about".
mjs remains the default, and the majority of the complaintants either lacked understanding of the nuances or wanted to discard critical use cases. I do not think it is at all appropriate to paint this as "you yelled at us, we were wrong, and we fixed it! (even though we didn't actually change any of the things you yelled about"
(I stopped reviewing halfway through because the overall messaging is very dismissive of the real and valid reasons mjs is explicitly necessary and has always been)
doc/announcement.md
Outdated
|
||
## `import` and `export` syntax in `.js` files | ||
|
||
Yes, we heard [you](https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md). And [you](https://gist.github.com/ceejbot/b49f8789b2ab6b09548ccb72813a1054). And [you](https://twitter.com/maybekatz/status/1062473765865512961). You can now use `import` and `export` syntax in `.js` files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit disingenuous, since that wasn't prevented before - it just was not, and still is not, the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this something you can "now" do? This was something that was always planned; whether it happened to be shipped prior to the WG forming or not.
I do not want any implication that having an ESM .js
file is suddenly an option because of this working group; we have merely preserved (and perhaps implemented) that option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The article is describing the new implementation as compared to the previous implementation. It’s not describing the new implementation’s plan as compared to the possibly not fully implemented plan for the previous implementation. I don’t think most people will know (or care) what the previous plan was, just what’s new in this version of the software versus the previous version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the "we heard you" rhetoric at the beginning of the paragraph, this is absolutely implying that it wasn't part of the plan from day one, which it was. If you're talking about implementation differences and capabilities, then talking about "responding to feedback", especially flippantly implying that those of us involved prior to the WG a) weren't listening, or b) weren't planning on enabling this anyways, doesn't belong here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is absolutely implying that it wasn’t part of the plan from day one, which it was.
If supporting ESM in .js
was ever the plan, shipping without any support for it certainly sent a message to the community that that wasn’t the final plan. I was an interested observer when the first --experimental-modules
came out, probably a lot more interested than most, and I had no idea that ESM in .js
was ever on Node’s roadmap.
Regardless, plans are never mentioned here. This text is just saying that we hear the request from the community loud and clear, and we’re responding to it. That’s not an indictment of anyone involved in the process so far. If anything, it shows that we’re good developers who are working hard to build the best product for our users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not OK with the wording in this paragraph. I'm not sure how to explain it any clearer, but please stop marking it as resolved; it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of tone, I think it has much the same issue as the "we didn't do this to annoy you" line did. In isolation it might seem conciliatory, but mentioning it at all is only likely to incense anyone who feels these results are only a half-measure. It doesn't sound good to many of the people it's meant to ease the minds of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All we need is a simple statement that modules can use the JS extension.
That will be enough. It's super clear.
We should avoid referencing individuals that have taken particular positions in the past, because that frames it as a conflict between different sides, whereas it's more of a collaboration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, I think @ljharb's comments on the tone of the .mjs
bit are pretty valid.
@GeoffreyBooth reading over the rendered document, I have a more general consideration. People will likely lose track of which Does it make sense to always be explicit when first talking about either one in a separate context? ie always use the exact same way to refer to previous or new implementation since they are all flagged at this point. Suggestions: (use emojis)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for writing this up!
Let’s just settle this. The docs sub-group discussed and decided on the spelling “ES module” to match “CommonJS module”. If you agree with this, click 👍 below. If you prefer “ES Module” click 👎 below. Edit: The 👍s have it, as of this writing, so I’m going to mark as resolved all the “change spelling” requests. If the numbers flip, among members of the group, I’ll make the change. |
@GeoffreyBooth is that referring to a source text module or some abstract module (which could be a stm) |
Why do you ask, so you can point me to some spec text where everything is capitalized? This is a blog post, not spec text. An ES module is whatever the average user thinks it is, which I would think is probably a file or string containing |
@GeoffreyBooth because if its some abstract module i'd say "es module" (i don't care about the capitalization there) and if its a source text module i'd say "source text module" (we already refer to it as such in the vm api) |
cf24a46
to
6dee5cd
Compare
I think I’ve addressed all the feedback, at least that was clear on how I should address it. I’m not sure what changes to make regarding the dual packages/ |
doc/announcement.md
Outdated
|
||
## `import` and `export` syntax in `.js` files | ||
|
||
Yes, we heard [you](https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md). And [you](https://gist.github.com/ceejbot/b49f8789b2ab6b09548ccb72813a1054). And [you](https://twitter.com/maybekatz/status/1062473765865512961). You can now use `import` and `export` syntax in `.js` files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this something you can "now" do? This was something that was always planned; whether it happened to be shipped prior to the WG forming or not.
I do not want any implication that having an ESM .js
file is suddenly an option because of this working group; we have merely preserved (and perhaps implemented) that option.
Okay, I’ve addressed all of the feedback posted above, either through the revisions in the document or through replies in the comments. Some of the comment threads didn’t have clear direction for me to know what to do about, as I’ve written above; if people can come to a consensus on what we want to do regarding those parts of the document, I’m happy to make those changes. @MylesBorins plans to open the PR for upstreaming on Monday 2019-03-18, and I think this should be merged in before then so that his PR can link to this document at its final URL in the repo. We can continue to revise it before it’s published to the world as our announcement blog post. I understand some folks still disagree with some of the things I’ve written here; I would ask that we merge this in and you can propose alternate text in PRs against this document, and those PRs can be debated, possibly in a meeting. |
I'd prefer no announcement whatsoever over an announcement that conveys a flippant tone about |
I removed the entire paragraph that was criticized for its tone. I don’t feel that the remaining very brief mention of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments. Let me know if I can clarify further at all.
33c2a37
to
1116d7d
Compare
57599f4
to
4be23c8
Compare
|
||
## `import` and `export` syntax in `.js` files | ||
|
||
We heard some [very](https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md) [strong](https://gist.github.com/ceejbot/b49f8789b2ab6b09548ccb72813a1054) [feedback](https://twitter.com/maybekatz/status/1062473765865512961) that Node.js needs to provide a way to use `import` and `export` syntax in `.js` files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is much better - I'd still prefer to indicate that this was always part of the plan (just was never planned to be the default, as it still is not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This language was the result of a lot of back and forth between various people, so I'd rather not open it up again for debate if you don't mind? Could this be acceptable to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why "how many people that weren't me" you talked to should mean that I don't get to retain my opinion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the previous implementation’s user-facing docs and design doc, the previous plan for supporting ESM in .js
files was via a loader as shown in the example here.
How about we change “Node.js needs to provide a way to use import
and export
syntax in .js
files” to “Node.js needs to provide a way to use import
and export
syntax in .js
files without requiring a loader.” Would that be acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guy and myself had plenty of talks about different ways to declare formats prior to the wg forming, if you need me to dig up gists etc. I can, but the talks did occur both in EP and after the initial implementation. I am rather busy but is it really necessary to bicker about the timelines so much?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That suggestion which I strongly recommend based on the cases made in my previous outdated one would look more like this:
We heard some [very](https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md) [strong](https://gist.github.com/ceejbot/b49f8789b2ab6b09548ccb72813a1054) [feedback](https://twitter.com/maybekatz/status/1062473765865512961) that Node.js needs to provide a way to use `import` and `export` syntax in `.js` files. | |
We are make a lot of progress towards some [popular features demanded by members of the community](https://twitter.com/maybekatz/status/1062473765865512961) and in ways that will ensure that all existing projects can continue to operate smoothly. This required a lot of careful consideration and insights from community discussions on how [the community uses ESM in projects today](https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md) and [possible ways for packages to move forward](https://gist.github.com/ceejbot/b49f8789b2ab6b09548ccb72813a1054). | |
With this new implementation, we are making it possible for the first time to actually `import` and `export` from `.js` files with opt-in package configuration or CLI flags directly, ie no longer requiring a custom loader. |
Preview
We are make a lot of progress towards some popular features demanded by members of the community and in ways that will ensure that all existing projects can continue to operate smoothly. This required a lot of careful consideration and insights from community discussions on how the community uses ESM in projects today and possible ways for packages to move forward.
With this new implementation, we are making it possible for the first time to actually
import
andexport
from.js
files with opt-in package configuration or CLI flags directly, ie no longer requiring a custom loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because nobody insisted we would never do that. With or without any of this feedback, we would always have provided some way to have ESM in .js - predating the WG as well. Thus, the links aren’t relevant, and would further the misconception that this is a newly intended feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb can you clarify what the intent was:
- Allow a
package.json
to make it possible tonode main.js
to be ESM: Yes/Loader/No - Allow a flag
node --flag main.js
to be ESM: Yes/Loader/No - Allow a package to be published with
.js
files as ESM: Yes/Loader/No - Allow a package to be published with
.js
files as ESM & CJS: Yes/Loader/No
Can you reply on each one to help me get a more complete picture please.
Because from the end user's side (which I was at roughly 12 months ago) things looked a certain way, and regardless of what the intent was, it is important to recognize that no matter how hard we all try, the other perspective is always very hard fully appreciate — and naturally mine was one of a the wide spectrum of perspectives, but not an outlier
As such, it is important for us to be as objective and thorough as possible when talking to the audience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- afaik undiscussed, but i don't see any reason why we wouldn't have - if package.json has any control then it should have entry point control.
- yes, since day one, if for nothing else for stdin/eval/extensionless files.
- an open question, but leaning towards yes because if it can work for non-bare imports via package.json (ie, for your own top-level files) than it wouldn't make sense to actively prohibit it for published files.
- dual packages have always been intended; without that as an option, there's no migration strategy.
It is totally valid that some user perspectives may have been solely informed from the implementation; it's also valid that user perspectives do not get to dictate what the intention was.
In general, being objective also means not validating "internet mob outrage made change happen" so as to ensure that's not seen as an effective means to effect change in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying
Would a better path forward be to remove any language that has any sort of
bias... Avoid history... Talk about facts of new implementation only
…On Sun, Mar 24, 2019, 12:54 PM Jordan Harband ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/announcement.md
<#291 (comment)>:
> +
+- `import.meta.url` provides the `file://` URL of the current ES module file.
+
+- Loaders can be written to modify Node.js’s runtime behavior with respect to ES modules. _This is still very much a work in progress._
+
+- Node.js can be run with an ES module file as a program’s initial entry point.
+
+- Files loaded as ES modules are loaded in strict mode, which in CommonJS requires adding `'use strict';` to the top of every file.
+
+- Files ending in `.mjs` are explicitly treated as ES modules in `import` statements and when run via the `node` command.
+
+And the new version of `--experimental-modules` adds:
+
+## `import` and `export` syntax in `.js` files
+
+We heard some [very](https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md) [strong](https://gist.github.com/ceejbot/b49f8789b2ab6b09548ccb72813a1054) [feedback](https://twitter.com/maybekatz/status/1062473765865512961) that Node.js needs to provide a way to use `import` and `export` syntax in `.js` files.
No, because nobody insisted we would never do that. With or without any of
this feedback, we would always have provided some way to have ESM in .js -
predating the WG as well. Thus, the links aren’t relevant, and would
further the misconception that this is a newly intended feature.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#291 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAecV5sdcltblx2eS79nXXZEA0VVKs05ks5vZ624gaJpZM4bsNum>
.
|
I think so - I think this language achieves that. |
@GeoffreyBooth and I discussed things a bit offline; if by tomorrow night the two of us haven't been able to agree on better text, I'm going to go ahead and stamp with the current phrasing (also OK with this tweak, but not required). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can always iterate per-publish post-merge, if necessary, but this seems fine to me after numerous discussions.
Thanks to everyone for your patience and tolerance.
In all seriousness, I have heartfelt appreciation for all the careful feedback that everyone has given this, and I’m very encouraged that we were able to reach consensus. Thank you to everyone who took the time to comment or review. That said, I’m gonna merge before anyone else can object 😉This isn’t truly final until it’s posted on Medium, which I assume won’t happen until after Node 12 ships, so there’s still time for more revisions via PR between now and then. |
P.S. Just realized I shouldn’t delete the branch while there still might be links to it, such as in our upstream PR. If people could update wherever they might’ve posted the link to use the new URL, that would be great: https://github.com/nodejs/modules/blob/master/doc/announcement.md |
Here’s a draft of what our announcement blog post could be.
https://github.com/nodejs/modules/blob/master/doc/announcement.md