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

Strict ES Module Support #938

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft

Strict ES Module Support #938

wants to merge 16 commits into from

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Jul 26, 2023

Rendered

This RFC is about removing all the vestiges of our system that are not compliant with ES modules. The tl;dr is that require and define need to go away.

As a replacement for some of what people do with those APIs, I also submitted a companion RFC #939 to introduce import.meta.glob.

@github-actions github-actions bot added the S-Proposed In the Proposed Stage label Jul 26, 2023

This is very bad, because
- our code breaks when you try to run it in environments that actually follow the spec.
- refactors of Ember and its build pipeline that would otherwise be 100%
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

I super appreciate the callout to, "if we follow the spec", upgrades will be easier and more stable for consumers


## Recommendations: Replacing `require` in Addons

- If you're trying to import Ember-provided API that is only available on some Ember versions, use `dependencySatisfies` to guard the `import()` or `importSync()`.
Copy link
Sponsor Contributor

@NullVoxPopuli NullVoxPopuli Jul 27, 2023

Choose a reason for hiding this comment

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

should caveats be added here that folks need to get their dep graph under control in some scenarios? (in particular for dependencieSatisfies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I said that explicitly in the require.has section but not this section.

Copy link
Sponsor Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

very excited for this, long-awaited, much needed deprecation

@ef4 ef4 marked this pull request as draft July 28, 2023 16:34
@ef4
Copy link
Contributor Author

ef4 commented Jul 28, 2023

Converted back to draft because this needs to say a lot more about Resolver.

@simonihmig
Copy link
Contributor

Converted back to draft because this needs to say a lot more about Resolver.

I was going to ask about this. But also can you shed some light into the implementation aspect of this. I know the RFC does not have to be prescriptive here, but maybe broadly indicate what needs to be done to implement it. Like is the final publishing format going to be ES modules? Or are we still using something AMD-like under the hood, just making it truly an implementation detail by not leaking it? How will importSync be able to work?

FWIW, I just closed #784 in favor of this, but can you maybe also look into the "Motivation" examples in that RFC and see if they are going to be covered by this RFC. You say that require and define will be undefined, but will that also guarantee that any external definition of these (3rd party AMD scripts, node's CJS) will not cause any conflicts anymore?

@ef4
Copy link
Contributor Author

ef4 commented Jul 29, 2023

But also can you shed some light into the implementation aspect of this. I know the RFC does not have to be prescriptive here, but maybe broadly indicate what needs to be done to implement it.

Yes, I think you're right and this RFC in particular really need to explain the impact at both the high-level API layer and the deeper details, because they are currently not hidden very well.

FWIW, I just closed #784 in favor of this, but can you maybe also look into the "Motivation" examples in that RFC and see if they are going to be covered by this RFC. You say that require and define will be undefined, but will that also guarantee that any external definition of these (3rd party AMD scripts, node's CJS) will not cause any conflicts anymore?

Oh yes, this is a very good point. I do think this RFC should solve those problems, and I will make it say something about that explicitly. And that adds some good constraints to the design. It basically means nothing should use shared globals to talk about "the" ember app.

@runspired
Copy link
Contributor

Overall hugely looking forward to this!

I wish we'd pushed towards https://github.com/stefanpenner/ember-strict-resolver ages ago to make this a bit easier for some of the weird edge cases.

I suspect the sometimes tricky details around classic vs pods resolution are one we can probably absorb for now. For models, adapters, and serializers EmberData currently normalizes prior to lookup and has deprecated non-strict lookup strings. There are some exceptions to this normalization though and I'll be sure to look to address those. The general path for EmberData is to stop using the resolver entirely as it is, none of our newer patterns encourage or requires it and even adapters/serializers/models can be made to work without it.

The trickier cases I suspect are things like the normalization of lookup/factoryFor/service strings / keys to module names. It's been valid to pass in AnYcRaezYString and expect to get back matches for an-yc-raez-y-string etc.

@ef4 ef4 added S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Aug 4, 2023
@ef4
Copy link
Contributor Author

ef4 commented Aug 4, 2023

Move to exploring in RFC review meeting just now.

@Windvis
Copy link

Windvis commented Aug 8, 2023

This will most likely break the ember inspector dev tools. Is that a blocker for this RFC?

@achambers
Copy link
Contributor

This will most likely break the ember inspector dev tools. Is that a blocker for this RFC?

@Windvis Yep, ember dev tools need to keep working in order for this to move to Released.

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented Aug 18, 2023

I'm a little doubtful this is the right place to comment, but

I ran in to something that this RFC maaaybe unblocks yesterday with @runspired

changes here: https://github.com/NullVoxPopuli/library-provides-web-worker-in-ember-project/pull/1/files

the tl;dr: is that we can't utilize ESM Web Workers (via auto-import in classic builds), unless all of ember-source is non-AMD?

The error from the PR

image

image

@ef4
Copy link
Contributor Author

ef4 commented Sep 12, 2023

A TODO: This should probably say more about engines.

@ef4
Copy link
Contributor Author

ef4 commented Oct 13, 2023

We are actively working through making ember's internals follow the proposed rules in here as a first step before we finalize the RFC and tell everybody else to update their code in this way.

ef4 added a commit to glimmerjs/glimmer-vm that referenced this pull request Dec 24, 2023
While working toward [strict-es-modules](emberjs/rfcs#938), I found that under real ES modules, this feature-detection code will become eager, which makes it blow up inside all versions of ember-cli-htmlbars before ember-cli/ember-cli-htmlbars#786, because ember-cli-htmlbars insists on evaluating the template compiler bundle inside a VM with no valid `URL` global.

While that is now fixed in an upcoming major of ember-cli-htmlbars, people can't benefit from that until their very last addon upgrades ember-cli-htmlbars.

So it's also good to fix this directly, by making the feature detection code explicitly lazy.
ef4 added a commit to glimmerjs/glimmer-vm that referenced this pull request Dec 24, 2023
While working toward [strict-es-modules](emberjs/rfcs#938), I found that under real ES modules, this feature-detection code will become eager, which makes it blow up inside all versions of ember-cli-htmlbars before ember-cli/ember-cli-htmlbars#786, because ember-cli-htmlbars insists on evaluating the template compiler bundle inside a VM with no valid `URL` global.

While that is now fixed in an upcoming major of ember-cli-htmlbars, people can't benefit from that until their very last addon upgrades ember-cli-htmlbars.

So it's also good to fix this directly, by making the feature detection code explicitly lazy.
ef4 added a commit to glimmerjs/glimmer-vm that referenced this pull request Dec 24, 2023
While working toward [strict-es-modules](emberjs/rfcs#938), I found that under real ES modules, this feature-detection code will become eager, which makes it blow up inside all versions of ember-cli-htmlbars before ember-cli/ember-cli-htmlbars#786, because ember-cli-htmlbars insists on evaluating the template compiler bundle inside a VM with no valid `URL` global.

While that is now fixed in an upcoming major of ember-cli-htmlbars, people can't benefit from that until their very last addon upgrades ember-cli-htmlbars.

So it's also good to fix this directly, by making the feature detection code explicitly lazy.
ef4 added a commit to emberjs/ember-cli-babel that referenced this pull request May 24, 2024
For bad historic reasons, if you disable AMD compilation with `compileModules: false`, it opts you into the using the `Ember` global via modulesAPIPolyfill and debug-macros. This is a problem because we absolutely *do* want to disable AMD compilation as we move toward [strict-es-modules](emberjs/rfcs#938), but the Ember global has been gone since 4.0.

It's possible to work around the modulesAPIPolyfill issue by explicitly turning the polyfill off. But it's impossible to work around the debug-macros problem, because we don't want to turn them off, we want to keep them on but tell them to use imports instead of globals.

In both cases, it's a bad default that emits code that could never possibly work. This PR makes Ember 4.0 the hard cutoff, after which we will never emit globals, because they don't work there anyway.
@ef4
Copy link
Contributor Author

ef4 commented Jun 14, 2024

Thanks but I'm not ready for Accepted stage yet. This needs edits and is still in draft.

@ef4
Copy link
Contributor Author

ef4 commented Jun 14, 2024

To summarize the status: I've been doing the implementation testing for this and think that the importable backward-compatibility modules in this RFC won't work as written due to deadlock concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Exploring In the Exploring RFC Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants