Skip to content
This repository has been archived by the owner on Jul 31, 2018. It is now read-only.

Node v2.0 modules #43

Closed
wants to merge 7 commits into from
Closed

Conversation

Fishrock123
Copy link

@Fishrock123 Fishrock123 commented Sep 8, 2016

My, possibly transitional, "counter-proposal" to #39.

Mostly hashed out, some details may be missing as this style of thing is not my forte.
This is what is roughly described in a recent meeting with other CTC members.

It is driven by an interest of our existing ecosystem, to maintain as much compatibility with everyone's existing hard work as possible, while allowing newer functionality to be used without major consequence.

Like many other things in node core, it is a compromise. A compromise from idealism to better suit the reality of our platform.

cc @bmeck, @ljharb, @nodejs/ctc i guess

Dear lord I hope this doesn't gather a ton of hate.

Edit: this is rebased on #39 for some language simplicity reasons. I can probably merge those directly into his commit though.


The RMR poses several interoperability and tooling problems:

1. Importing from NCJS has no support for Named Imports.
Copy link
Member

Choose a reason for hiding this comment

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

this is unfortunate :-( i think it's an OK thing to sacrifice, but basically everyone using babel right now to named-import from CJS is going to break.

Copy link
Author

Choose a reason for hiding this comment

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

Note: this is a problem with using the RMR which this proposal avoids, these are not problems with this proposal. :)

(Also saying for anyone else reading this.)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, understood. Thanks for clarifying!

@dinoboff
Copy link

dinoboff commented Sep 8, 2016

I don't really see the point.

If async loading is such an issue, I would prefer to have CJS scripts not able to load modules than to have half implemented ES modules. ES module developers would still need to publish a transpiled version but they would be able to run their module natively.

@Fishrock123
Copy link
Author

Fishrock123 commented Sep 8, 2016

@dinoboff See the discussion at #43 (comment)

In summary, new ES Features will likely only target the Module Parse Goal, and as such it is a good idea to make it available, even if it is not within ES Modules due to problems with them.

half implemented ES modules

Hmmm 💭 I don't think this ever claimed to be that.

This claims to implement the Module Parse Goal while specially not implementing ES Modules in the loading / import / export sense, since there is many problems with that.

@Fishrock123
Copy link
Author

Updated with some more problems with Spec ES Modules and addressed some comments.

@Fishrock123
Copy link
Author

Also a note which I don't know where to put yet: This retains "top-level" return, which is important in many Node scripts.

@ljharb
Copy link
Member

ljharb commented Sep 8, 2016

That's not part of the module goal though. There isn't supposed to be a top-level return, and I'm not sure why that's valuable in CJS modules anyways.

@Fishrock123
Copy link
Author

Updated with an example for ### 1.2. Avoidance of Async Module Resolution.

@dinoboff
Copy link

dinoboff commented Sep 10, 2016

@Fishrock123 Node could support three types:

  • cj scripts - parsed in strict mode using legacy loader.
  • cj modules - parsed in module mode using legacy loader.
  • es modules - parsed in module mode using System loader.

With the two loaders supporting or not each other.

This proposal doesn't need to be a rewrite of 002-es-modules; why not a 004-cj-modules proposal?

@Fishrock123
Copy link
Author

@dinoboff unsure if that would be useful.

@Qard
Copy link
Member

Qard commented Sep 11, 2016

If we can get top-level await, we could just do something like await Promise.resolve(exports) inside the require function, making the asynchrony transparent to existing modules with the sync expectation.

@bmeck
Copy link
Member

bmeck commented Sep 12, 2016

@Qard not sure I understand, await would still only be available within async contexts (Async functions / Module scope). Any context using it would need to provide an async wrapper (Promise) as await only pauses the current stack frame.

@Qard
Copy link
Member

Qard commented Sep 12, 2016

I understood top-level async/await as having await everywhere and, when used outside an async function, would halt the entire JS thread. Perhaps I misunderstood it, but it seemed like a lot of people had the same understanding of it.

Alternatively, a function on promises to block until resolution would help too, like a thread join. Though I'm not sure how that'd impact other things in the microtask queue, and I somehow doubt TC39 would be into that idea...

@loganfsmyth
Copy link

I understood top-level async/await as having await everywhere and, when used outside an async function, would halt the entire JS thread. Perhaps I misunderstood it, but it seemed like a lot of people had the same understanding of it.

top-level await is specifically about allowing await in the "top level" scope of a module (outside all functions), it would still never be usable inside arbitrary functions. If a module is mid-evaluation and hits an await, it will suspend execution of the module body until the promise resolved, but the JS thread would not be halted and other JS could be executed while the module body was waiting, the same way an async function would.

@Jessidhia
Copy link

There could be the option of having a syntax extension where a call to require would have an implicit await in front of it.

It would still not be completely transparent, though, as it'd break requires not called at the top level or in async functions.

@Qard
Copy link
Member

Qard commented Sep 13, 2016

Ah, in that case...IGNORE ME.

@Fishrock123
Copy link
Author

closing, agreement has come around to other options

@Fishrock123 Fishrock123 closed this Mar 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants