Introduce `no-globals` optional feature #784

simonihmig · 2022-01-07T11:48:41Z

For the record: there has been an early draft and some discussion around it here: #781. Special thanks to @bendemboski for contributing a lot of details about the ember-electron use case!

rwjblue

Broadly, this seems great! I've left a few inline comments, but overall I'd like to see us move away from requiring the IIFE solution (in favor of providing an API more broadly that folks can leverage to do the app's own require/define as needed directly).

rwjblue · 2022-04-25T16:23:05Z

text/0784-no-globals.md

+* naming global variables in a unique way, e.g. scoped to the app's name (in `package.json`), so that two different Ember
+  apps cannot cause conflicts


Can you describe how this would work RE: ember-engines (where applicable)? I don't think ember-engines itself will have an issue here, but I could absolutely imagine specific engines needing/wanting some global coordination; should they use ${appName}-${engineName}?

Tbh I am not very knowledgable about engines. What do you mean with ${appName}-${engineName}, does that refer to the renamed version of require? Why would they need any special coordination?

In general, folks should think of an engine as another app (in that it's self contained/isolated). I was mostly wondering if we should advise folks to scope engine related globals to the engine name or to a combination of the app name and the engine name.

Totally fine to not have an answer, I don't think we have to have concrete guidance here for that (we can make an issue over on ember-engines when this RFC merges)

rwjblue · 2022-04-25T16:24:34Z

text/0784-no-globals.md

+* ideally also adding a non-deterministic part like a random string that changes *per build*, that is only known to
+  privileged code (as listed above), so user-land code cannot rely on the existence of these globals.


I like this alot, we should provide a mechanism to actually do this conveniently (it's probably already possible to leverage @embroider/macros's setConfig in some combination?)

rwjblue · 2022-04-25T16:27:27Z

text/0784-no-globals.md

+
+The global functions/namespaces `define` and `require` (and its aliases `requireModule`, `require`, `requirejs`) are set
+as globals and provided by the [`loader.js`](https://github.com/ember-cli/loader.js) addon, which is part of the default
+blueprint of any Ember app. Not having this addon or any equivalent replacement will break the app, as a.o. any `define()`


as a.o.

I'm sorry, I'm not familiar with this abbreviation; can you expand?

Oh, it was meant to mean "amongst others", but I don't know if that is a valid abbreviation at all. 😅. Will expand that phrase...

rwjblue · 2022-04-25T16:28:31Z

text/0784-no-globals.md

+Despite that, Ember comes with its own (stripped down) [loader library](https://github.com/emberjs/ember.js/blob/master/packages/loader/lib/index.js),
+that has been used (pre Ember 3.27) internally only. As of Ember 3.27 it is effectively unused when `loader.js` is present
+(which it has to be, see above), making it dead code.


FWIW, I think we can remove https://github.com/emberjs/ember.js/blob/master/packages/loader/lib/index.js completely now (as of Ember 4.0.0).

rwjblue · 2022-04-25T16:32:50Z

text/0784-no-globals.md

+<summary>vendor.js</summary>
+
+```diff
+ (function(){


I don't think we actually want to wrap all of vendor.js in an IIFE here, I'd much prefer to change Babel compilation to use the alternate function names (and still have them dynamic by app name like you suggest).

Can you explore that and see if that is plausible?

But wouldn't that break any addon code that still uses e.g. require()?

The idea here was to prevent leaking those into the global namespace, while still allowing all existing addon code that uses these things in the wild (most notably ember-resolver) to still work. That is as long as that addon code is processed through the normal Ember CLI build process, i.e. it lands within the IIFE. When you call require from somewhere else like an external <script>, that would cease to work, hence the optional feature to allow this "intimate API" to break in such rare cases.

Note that I am referring to addons directly calling require() etc., like here, and not regular ESM imports that happen to be transformed to define/require during the build.

Or do you suggest to transform any addon code that uses require() directly to refer to the renamed version of it?

Also note that I mentioned under "Alternatives" that we could also drop all support of user-land code calling the AMD functions. Which would be more invasive, breaking some addon's code. Still no breaking change, as that is opt-in only.

Then indeed we don't need that IIFE nor any addon code transformations...

But wouldn't that break any addon code that still uses e.g. require()?

Yes 😈 , but that's the point of creating an optional feature anyways (it's opt-in, we are allowed to limit semantics).

The idea here was to prevent leaking those into the global namespace, while still allowing all existing addon code that uses these things in the wild (most notably ember-resolver) to still work. That is as long as that addon code is processed through the normal Ember CLI build process, i.e. it lands within the IIFE. When you call require from somewhere else like an external <script>, that would cease to work, hence the optional feature to allow this "intimate API" to break in such rare cases.

Ya, I think we should provide a global that is app scoped (e.g. globalThis._appNameGlobals.require and globalThis._appNameGlobals.define) instead of continuing to use define/require unscoped. This would require some tweaks to the babel setup (e.g. we'd need a custom version of https://github.com/babel/babel/blob/main/packages/babel-plugin-transform-modules-amd/src/index.ts or send a PR to allow the define/require hard coded constants to change).

Note that I am referring to addons directly calling require() etc., like here, and not regular ESM imports that happen to be transformed to define/require during the build.

I actually think this case is not a good example. Specifically because that is not using require global, it is using import require from 'require';:

https://github.com/miragejs/ember-cli-mirage/blob/d99571d9010011976b757f40cca8450c71dcb070/packages/ember-cli-mirage/addon/utils/read-modules.js#L8

^ pattern should still continue to work just fine (because whatever loader happens to be running, will respond to that import with the correctly scoped require).

Yes 😈 , but that's the point of creating an optional feature anyways (it's opt-in, we are allowed to limit semantics).

Yes, which is basically what I say here, right? 👇

Also note that I mentioned under "Alternatives" that we could also drop all support of user-land code calling the AMD functions. Which would be more invasive, breaking some addon's code. Still no breaking change, as that is opt-in only.

At least when they are called as globals. Which the current form of the RFC still tries to support...

So to get this 100% clear: you are suggesting that we do this alternative approach, i.e. stop supporting the global use of require() & Co as globals all together, right?

This does mean that we need to "fix" a lot more of the core addons, like ember-resolver, which AFAICT does use require() as a global here

I actually think this case is not a good example. Specifically because that is not using require global

Oh, you are right, I missed that! It's not so easy to search for examples of addons doing require() using emberobserver's code search, given that you get a bazillion hits for legit use of require() in node-land... 🙈

pattern should still continue to work just fine (because whatever loader happens to be running, will respond to that import with the correctly scoped require)

While that is possible, I wonder if we actually want that. Probably out of scope for this RFC, but shouldn't we eventually try to get rid of any usage of loader.js stuff at all?

Like in a future where we have Polaris and Embroider by default, shouldn't we tell users that the only way to import stuff is via ESM imports (and @embroider/macros)? So that we can eventually get to a point where we could get rid of any specific runtime module system like loader.js, when we have just ESM imports at stage 3, then throw that at webpack, which then emits whatever it emits.

So maybe turning on no-globals is actually a good moment to make all of require() stop working for unprivileged code?

rwjblue · 2022-04-25T17:10:15Z

text/0784-no-globals.md

+into account. Implementation wise this can happen in the same way as described above, by defining `EmberENV` within the
+IIFE (and not using the explicit `window.` namespace).


I think it would be better to have Ember leverage @embroider/macros getConfig or getOwnConfig inside Ember's own code where we currently read window.EmberENV instead of leveraging the IIFE style.

It seems fine to still support reading from window.EmberENV even in no-globals mode? Otherwise we need to propose something for the Ember Inspector to enable the debug render tree.

Actually, come to think about it, I think the inspector in its current form/architecture will most likely just completely break in no-globals mode as it relies on being able to get access to the things you are trying to no leak with this RFC. Probably not a good enough reason to hold this back, but it's definitely a drawback that should be noted. We can overhaul the inspector in theory but it will require more design than is in this RFC currently, and it's probably not fair to have this RFC solve that problem, plus I am not sure if anyone has time to work on a big inspector project. In the short/medium term, I think the inspector will probably just not work with no-globals mode. Is that an acceptable drawback?

(cc @rwwagner90)

Oh, that's a good point! I didn't think about that. Certainly needs to be mentioned. I don't know enough about the Inspector to assess how much needs to change to support no-globals. I guess all those possible ways to coordinate between those privileged tools what the scoped version of require() & Co are (e.g. getConfig) will not work for the Inspector, as it is a standalone app that does not take part in the build system of the app...

Maybe there still could be some escape hatch like defining window.__ember_scoped_globals_only_for_inspector_dont_ever_use_this_in_user_code = 'myEmberApp_somerandomhash', when the app has require (and define) scoped as window.myEmberApp_somerandomhash.require?

Is that an acceptable drawback?

I would still say yes. You can still define the flag conditionally, like enable it for test and prod, while disabling it for dev!

simonihmig · 2022-04-25T18:23:56Z

overall I'd like to see us move away from requiring the IIFE solution

See my other inline comment about this. The idea was to let any direct use of require() continue to work...

(in favor of providing an API more broadly that folks can leverage to do the app's own require/define as needed directly).

Not sure I get what you mean here. Who is "folks"? Like I would see user-land code to only use ESM, and eventually @embroider/macros (like importSync()) for the special cases. But not expose anything new here as a public API for users.

Or do you refer to what I called the privileged addons, so they use some private API to get references to define/require? But again, this would require either breaking user-land code, or transforming it also. The IIFE was meant to let user code continue to work, without transformation...

Which brings me to the overarching question of how deep we should start to dig into the implementation details of this RFC. I did all this IIFE exploration only to prove that it's feasible to make this work somehow (I literally changed the /dist code by hand to see that the approach works), while intentionally stating in the RFC that those details are not normative. The TLDR of the publicly visible changes for users is basically: there is a new optional feature, that when enabled prevents leaking globals. For users everything should continue work, except for some exotic edge cases (code that is not processed through the normal build pipeline and which is relying on those globals). Everything else are implementation details.

Or do you think the RFC must specify the actual implementation path?

rwjblue · 2022-04-25T21:24:44Z

Not sure I get what you mean here. Who is "folks"? Like I would see user-land code to only use ESM, and eventually @embroider/macros (like importSync()) for the special cases. But not expose anything new here as a public API for users.

Ya makes sense! I think I agree with you that 99.9% of users won't have to ever care what we name the app specific globals (from my comment above that might be globalThis._appName.require/globalThis._appName.define), but I do think it should be possible for that 0.01% case to be written. However, I think my other comments address this as well: users in that edge case can leverage getConfig to find out the scoped property name.

Which brings me to the overarching question of how deep we should start to dig into the implementation details of this RFC.

I think that we need to explain enough of how it will work for folks to be able to have a mental model. In my mind that "simple" mental model is that when you opt-in to the optional feature, we don't use define/require globals at all and instead we put those in a scoped globals bucket for the app itself (like I mentioned above). We don't have to get into all the places that will need to be updated/fixed for that to work though.

simonihmig · 2022-04-26T15:40:55Z

but I do think it should be possible for that 0.01% case to be written.

Can you give an example of a legit use case where you would absolutely need this, and cannot express this in terms of regular imports or the importSync macro?

If we make this possible for unprivileged code, aren't we then basically introducing a new public API, that we need to support? Or at least kind of replacing one intimate API with another one? See also my other inline comment about whether to support import require from 'require'...

users in that edge case can leverage getConfig to find out the scoped property name

Speaking of the implementation detail of how to get hold of the scoped version of define and require, are you sure we actually need a "runtime" (in the sense that it goes into app code) thing like getConfig? I was rather thinking about something that is available for the build system, i.e. in node.js. So something that you could use for example in ember-auto-import here.

wagenet · 2022-07-25T14:00:24Z

It looks like this still has a good bit of interest so I'll see what I can do to keep it moving forwards.

ef4 · 2023-02-01T18:23:56Z

I think the framing of this RFC is too focused on what not to do ("don't use globals") when the problem would also go away if we clarify what to do instead.

I would rather solve the same problems by clarifying exactly how does one boot an Ember app, starting from only web standards like ES modules. It's a problem that our boot process is currently implementation-defined. This RFC is about changes in that implementation. But the implementation details should be invisible and shouldn't require an RFC at all! We need to get the implementation details behind public API and web standards.

jamesarosen · 2023-06-01T21:46:39Z

We need to get the implementation details behind public API and web standards.

I'm interested to hear what standards you might have in mind. I haven't seen any for "how to boot an application within the DOM." The closest things I can think of are

Web Components, which defines constructor, connectedCallback, disconnectedCallback, adoptedCallback, and attributeChangedCallback as the lifecycle API. I think it's architecturally elegant to think of a whole Ember application as a single web component, though there are likely technical limitations to the approach. We could adopt the lifecycle API without actually making the Ember application a true Web Component, setting the stage for a standards-track WebComponentLifecycle interface.
iframe -- I'm certainly not proposing that Ember applications be forced into iframes, but the iframe is a standard unit of web composability and exposes known lifecycle APIs like DOMContentLoaded and BeforeUnloadEvent.
The single-spa application lifecycle, which defines bootstrap, mount, and unmount as the lifecycle API. single-spa isn't a web standard, but it is reasonably popular.

In any case, there would have to be some non-module entrypoint JS (<script type="text/javascript">) to kick things off. Everything after that could be a <script type="module">.

ef4 · 2023-06-01T22:19:16Z

In any case, there would have to be some non-module entrypoint JS (<script type="text/javascript">) to kick things off.

Why non-module? I suspect it can be all modules all the way down.

I'm interested to hear what standards you might have in mind.

Something like:

in an ES module
import a particular function from ember and call it with arguments that include
- an HTMLElement to render into
- some description of the ES modules that define your application, so that it can load them when it want to
- and maybe something like a Location that mediates all access to the current URL.

jamesarosen · 2023-06-01T23:04:21Z

Why non-module? I suspect it can be all modules all the way down.

You're absolutely right. <script type="module" src="/ember-entrypoint.js"> works great. I should have said there has to be a <script> tag to kick things off; everything else can be imported by URL or importmap.

Something like…

This is very similar to the single-spa API. Each application is defined as a single top-level module that exports { boostrap, mount, unmount }.

They are registered with the application loader with this API:

registerApplication({
  name: '@organization/app2',
  app: () => import('/src/app2/main.js'),
  activeWhen(location) => location.pathname.startsWith('/app2'),
  customProps(appName, location) => ({}),
);

or the HTML version of the same:

<script type="importmap">
  {
    "imports": {
      "@organization/app2": "/src/app2/main.js"
    }
  }
</script>
<route path="/app2">
  <application name="@organization/app2"></application>
</route>

simonihmig · 2023-07-29T10:07:47Z

I would rather solve the same problems by clarifying exactly how does one boot an Ember app, starting from only web standards like ES modules. It's a problem that our boot process is currently implementation-defined. This RFC is about changes in that implementation. But the implementation details should be invisible and shouldn't require an RFC at all! We need to get the implementation details behind public API and web standards.

Sure, I fully agree. But the goal of this RFC was not a full-scale major change, but rather a first stepping stone towards that future, like making the AMD implementation not leak into the outside world, while still preserving compatibility inside the Ember world. The use of AMD leaks so much into the ecosystem, that it is not really an implementation details as it should, but rises to the point of being a public API, that's why I introduced a first opt-in feature to scale that down by making it invisible from the outside world. I did also have in mind to maybe deprecate it as part of this RFC, but chose to leave that for a later RFC.

Having said that, I don't think it makes much sense to keep working on this RFC at this point, so I will close this in favor of #938, which I think goes far beyond what is proposed here, so that stepping stone is not really needed anymore. Even what that RFC might not cover the use cases brought up here (idk, does it?), and when to cover those we still need another RFC, it probably makes sense to define that one on top of #938.

simonihmig mentioned this pull request Jan 7, 2022

Ember builds without conflicts of globals #781

Closed

no-globals optional feature

e8d0944

simonihmig force-pushed the no-globals branch from 0e86e17 to e8d0944 Compare January 7, 2022 11:51

locks added the T-framework RFCs that impact the ember.js library label Feb 15, 2022

rwjblue reviewed Apr 25, 2022

View reviewed changes

rwjblue added T-infrastructure T-Tooling RFCs that impact tooling, like Ember Inspector T-ember-cli RFCs that impact the ember-cli library T-ember-engines RFCs that impact the ember-engines library labels Apr 25, 2022

rwjblue self-assigned this Apr 25, 2022

wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022

wagenet added S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Jan 27, 2023

simonihmig closed this Jul 29, 2023

simonihmig mentioned this pull request Jul 29, 2023

Strict ES Module Support #938

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `no-globals` optional feature #784

Introduce `no-globals` optional feature #784

simonihmig commented Jan 7, 2022 •

edited

Loading

rwjblue left a comment

rwjblue Apr 25, 2022

simonihmig Apr 25, 2022

rwjblue Apr 25, 2022

rwjblue Apr 25, 2022

rwjblue Apr 25, 2022

simonihmig Apr 25, 2022

rwjblue Apr 25, 2022

rwjblue Apr 25, 2022

simonihmig Apr 25, 2022

simonihmig Apr 25, 2022

rwjblue Apr 25, 2022

simonihmig Apr 26, 2022

rwjblue Apr 25, 2022

chancancode Apr 26, 2022

chancancode Apr 26, 2022

chancancode Apr 26, 2022

simonihmig Apr 26, 2022 •

edited

Loading

simonihmig commented Apr 25, 2022

rwjblue commented Apr 25, 2022

simonihmig commented Apr 26, 2022

wagenet commented Jul 25, 2022

ef4 commented Feb 1, 2023

jamesarosen commented Jun 1, 2023 •

edited

Loading

ef4 commented Jun 1, 2023

jamesarosen commented Jun 1, 2023 •

edited

Loading

simonihmig commented Jul 29, 2023

		* naming global variables in a unique way, e.g. scoped to the app's name (in `package.json`), so that two different Ember
		apps cannot cause conflicts

		* ideally also adding a non-deterministic part like a random string that changes per build, that is only known to
		privileged code (as listed above), so user-land code cannot rely on the existence of these globals.

		into account. Implementation wise this can happen in the same way as described above, by defining `EmberENV` within the
		IIFE (and not using the explicit `window.` namespace).

Introduce no-globals optional feature #784

Introduce no-globals optional feature #784

Conversation

simonihmig commented Jan 7, 2022 • edited Loading

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

simonihmig commented Apr 25, 2022

rwjblue commented Apr 25, 2022

simonihmig commented Apr 26, 2022

wagenet commented Jul 25, 2022

ef4 commented Feb 1, 2023

jamesarosen commented Jun 1, 2023 • edited Loading

ef4 commented Jun 1, 2023

jamesarosen commented Jun 1, 2023 • edited Loading

simonihmig commented Jul 29, 2023

Introduce `no-globals` optional feature #784

Introduce `no-globals` optional feature #784

simonihmig commented Jan 7, 2022 •

edited

Loading

simonihmig Apr 26, 2022 •

edited

Loading

jamesarosen commented Jun 1, 2023 •

edited

Loading

jamesarosen commented Jun 1, 2023 •

edited

Loading