Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Rewrite readme #482

Merged
merged 15 commits into from
Feb 22, 2020
Merged

Rewrite readme #482

merged 15 commits into from
Feb 22, 2020

Conversation

GeoffreyBooth
Copy link
Member

Resolves #473. This archives out-of-date content and puts on our README the most important information that most visitors to this repo are likely looking for.

README.md Outdated
* Browser equivalence ([#133](https://github.com/nodejs/modules/issues/133))
* Don’t break CommonJS ([#112](https://github.com/nodejs/modules/issues/112))
* No refactoring ([#87](https://github.com/nodejs/modules/issues/87))
CommonJS is not statically analyzable (that’s one of the primary benefits of `import`/`export` syntax) and therefore the exported names cannot be known to Node.js simply by parsing JavaScript source code: the code needs to actually be evaluated in order for Node.js to discover what identifiers get attached to `module.exports` for each file. But evaluating the CommonJS code in what the ECMAScript Modules spec calls the “linking” phase (when the module graph is determined) is not permitted; this would mean that CommonJS code is evaluated early, before any ES module code, whereas the spec defines the order of modules’ evaluation based on the depth of imports. This early evaluation presents other issues in that if a dependency gets converted from CommonJS to ESM, its order of evaluation would change in the user’s application; and early evaluation might trigger side effects if reading the values from `module.exports` were getters or proxies.
Copy link
Contributor

@guybedford guybedford Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and early evaluation might trigger side effects if reading the values from module.exports were getters or proxies.

This is a separate point that isn't about early evaluation. This is specifically about Object.values(module.exports) having side effects, whereas those side effects would never otherwise trigger.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rephrase the text so that it’s correct? Maybe offer a suggestion?

Copy link
Contributor

@guybedford guybedford Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CommonJS is not statically analyzable (that’s one of the primary benefits of `import`/`export` syntax) and therefore the exported names cannot be known to Node.js simply by parsing JavaScript source code: the code needs to actually be evaluated in order for Node.js to discover what identifiers get attached to `module.exports` for each file. But evaluating the CommonJS code in what the ECMAScript Modules spec calls the “linking” phase (when the module graph is determined) is not permitted; this would mean that CommonJS code is evaluated early, before any ES module code, whereas the spec defines the order of modules’ evaluation based on the depth of imports. This early evaluation presents other issues in that if a dependency gets converted from CommonJS to ESM, its order of evaluation would change in the user’s application; and early evaluation might trigger side effects if reading the values from `module.exports` were getters or proxies.
CommonJS is not statically analyzable (that’s one of the primary benefits of `import`/`export` syntax) and therefore the exported names cannot be known to Node.js simply by parsing JavaScript source code: the code needs to actually be evaluated in order for Node.js to discover what identifiers get attached to `module.exports` for each file. But evaluating the CommonJS code in what the ECMAScript Modules spec calls the “linking” phase (when the module graph is determined) is not permitted; this would mean that CommonJS code is evaluated early, before any ES module code, whereas the spec defines the order of modules’ evaluation based on the depth of imports. This early evaluation presents other issues in that if a dependency gets converted from CommonJS to ESM, its order of evaluation would change in the user’s application.
In addition to these issues of execution ordering, there is also the problem of reading those named exports. The way ES modules define their exports still means we would be applying an `Object.values(module.exports)` for all CommonJS modules as soon as they finish executing, in order to populate the ES module exports. This can lead to a number of problems:
* The triggering of getters and proxies (eg for dynamic generation / deprecation warnings) can throw errors or cause unwanted side effects.
* New exports added to `module.exports` after this time would never be available as named exports.
* Mutations to `module.exports` cannot be kept live with the ES module exports, unless we turn all CommonJS `module.exports` into a proxy. This in turn could have backwards compatibility issues.

README.md Outdated
* Loader hooks
* ESM and CommonJS interoperability
* Node.js and browser interoperability
* VM modules implementation

## Current Efforts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could include a Current Status section before all the experimental / new stuff, that captures what the group has already achieved.

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like a good idea, I'm just a bit unclear on what you have in mind. What would the different statuses be? Here's an idea I had for displaying different statuses of various features, it might be similar to what you're after.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall content is great, I'm just thinking perhaps we need to separate the README from the history.md from the roadmap.md. We also don't want to risk the README content going stale again too.

If we instead have more introductory information about the group and what it is and what it's done in the README, linking off to the other sections rather.

If we're looking to make specific advisories to users about the implementation status, perhaps the better place for that is the modules docs?

@GeoffreyBooth
Copy link
Member Author

The overall content is great, I'm just thinking perhaps we need to separate the README from the history.md from the roadmap.md. We also don't want to risk the README content going stale again too.

Wherever we put the “this is what we’re working on now” content, it will gradually become stale. I think it belongs near the top of the README, because that’s what the general public is coming to our repo for. That’s the information they can’t get anywhere else. If they want to know what we’ve accomplished, they can read the ESM docs.

If we're looking to make specific advisories to users about the implementation status, perhaps the better place for that is the modules docs?

We do this as well; see the loaders section with its plentiful warnings.

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Feb 12, 2020

Please take notice that there is a link in the following sentence of the introduction of the ECMAScript Modules docs page, which currently expects doc/plan-for-new-modules-implementation.md to exist in its current location.

Node.js contains support for ES Modules based upon the Node.js EP for ES Modules and the ECMAScript-modules implementation.

This PR, if merged, would move it to the archives causing that URL to turn up with nothing. I recommend we keep a page at doc/plan-for-new-modules-implementation.md (maybe for a few months) informing people that the page has moved and link to its new home in the archives.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this content is suitable for the readme.

README.md Outdated

## Current Efforts

> ### [Current Plan for ECMAScript Modules support in Node.js](./doc/plan-for-new-modules-implementation.md)
### To Do / In Progress
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be very blunt then, since you have asked me to be, I simply do not think this content is suitable for our readme. Since it is rapidly changing, likely to get out of date, and not the right information for someone "interested in the Node.js modules group".

As said before, I think we should separate this out into roadmap.md and previous-work.md.

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest commit in my PR to this branch may be what you're after.

GeoffreyBooth@eabe9c2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said before, I think we should separate this out into roadmap.md and previous-work.md.

Would it be better to have something like a project board for the roadmap? So it can properly link to issues/PRs, including automatic status updates as things get closed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkrems, the argument against that approach, which was brought up to me when I proposed the exact same idea to @GeoffreyBooth, is that these things need to be archivable and a more-static public record of them needs to be kept.

@GeoffreyBooth
Copy link
Member Author

@guybedford please review. This version just archives the features and old road map, without adding any new content. This will satisfy #473 while we figure out the best place to put that other content (like why named exports from CommonJS probably aren't coming soon).

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking on the feedback, looks great. Then we just need to find somewhere suitable to flesh out the full compatibility status in one place.

@GeoffreyBooth GeoffreyBooth merged commit 2575fa6 into nodejs:master Feb 22, 2020
@GeoffreyBooth GeoffreyBooth deleted the rewrite-readme branch February 22, 2020 05:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite modules readme
4 participants