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

Ember builds without conflicts of globals #781

Closed
simonihmig opened this issue Dec 16, 2021 · 11 comments
Closed

Ember builds without conflicts of globals #781

simonihmig opened this issue Dec 16, 2021 · 11 comments

Comments

@simonihmig
Copy link
Contributor

This is a draft of an RFC, for which I'd like to get some early feedback. Will then finalize and submit it as a real RFC.

Summary

Due to the use of the global namespace, especially for its AMD-style module system, Ember apps have problems coexisting
with other Ember apps or any AMD-based JavaScript on the same page. This RFC proposes a way to prevent all potentially
conflicting usage of these globals.

Motivation

The most common case for running an Ember app is to run it as a pure isolated SPA (Single Page Application), i.e. to have
it own the HTML page it runs on. This is what you get out of the box when running ember build: a (mostly empty)
index.html, that contains the build assets that run the Ember app inside that empty page.

But another use case is to embed it into an existing page,
so that it runs alongside other JavaScript (let's call it "3rd party" from the perspective of the Ember app). While that
might work fine in some cases, due to Ember's still present reliance on globals, especially its AMD-style module
system that is actually incompatible with the original one of RequireJS, there is a high chance of non-trivial conflicts.

Let's look at a few examples:

Embedding an Ember app into a 3rd party HTML page

You might want to embed an Ember app on a 3rd party page (i.e. one that you don't control), for example

  • a messaging/chat widget embedded on a corporate/sales website (think of Intercom)
  • a product configurator embedded on an e-commerce site (that's what we do)
  • a messaging board app on a customer support website (think of Discourse, but embedded)

In these cases, if the 3rd party page uses RequireJS itself, only one implementation (Ember vs. RequireJS) of the globally
defined define() and require() functions will "win", and the other's module loading will inherently be broken, in
the worst case both.

This is not uncommon, for example Magento, the most wide-spread e-commerce solution worldwide,
uses RequireJS, so effectively
you cannot use an Ember app on its pages without somehow tackling that conflict.

Micro Frontends

A rather new pattern in frontend development is the analogy of microservices for the frontend world, coined
Micro Frontends, where you develop, test and deploy parts of
your frontend independently, and compose them together for a seamless user experience.

The same issue arises when one of those micro frontends happens to use RequireJS, or two or more frontends are actual
Ember apps. While in the latter case we don't have to deal with different implementations of define() and require(),
those functions close over their module registry, so whatever functions win (as being the globally defined one), they will
only be able to see their registered modules, the modules of the other app will not exist anymore. And we are not even
speaking about conflicting versions of the same modules (say @ember/component). So in any way, that scenario is bound
to fail miserably.

Detailed design

The guiding principle suggested in this RFC is to make Ember apps practically side effect free, with respect to the
usage of the browser's global namespace (window/globalThis).

That means it should be discouraged to

  • read custom variables from the global namespace
  • write anything to the global namespace

While this can only be a recommended but not enforceable best practice for user-land code, for Ember and its build
tooling this should become a rule. That specifically includes:

  • Ember.js (its runtime code)
  • Ember-CLI
  • ember-resolver
  • ember-auto-import
  • Embroider

Where this is practically not possible, it should be ensured that there is no realistic chance of a conflict, by

  • 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.

While the Ember global has already been deprecated and removed from Ember 4 as part of RFC 706,
there are still other variables that Ember apps leak into the global namespace as described below, most notably Ember's
AMD-like module system, and as such pose a risk for conflicts.

Note that this RFC does not propose a new packaging format for Ember apps that does not rely on AMD-like modules.
While that could be a potential case for a future RFC, it is not in scope for this one. This only suggests to not rely
on the module system being accessed as globals, or at least as described above not in a way that poses a risk for conflicts.

Opt-in behavior

The changes proposed here should ideally only require changes in Ember's build tooling. For idiomatic Ember code, especially
code that only uses ES modules, it should be a non-breaking change. Also code that uses Ember's AMD-style module system
directly like require() or require.entries, should continue to work without any changes in most cases, as long as the
code is built by Ember-CLI (see below).

Although the AMD-like module API might not be strictly a public API of Ember, but it is often used in the wild to be at
least regarded as "intimate API". So for the scope of this RFC, we will consider it equivalent to public API.

But there might be cases where people are doing things differently, and code - even after the build tooling changes
proposed below - still effectively relies on the globally defined AMD-like module API. For this reason we may not just
remove these from the global namespace, breaking backwards compatibility.

So this RFC proposes an opt-in behavior, by introducing a new "feature" based on the already well known
@ember/optional-features, namely no-globals.

All the following proposals only apply when a user as explicitly opted into the new behavior, by enabling that optional
feature!

Note that similar to how Ember transitioned away from using jQuery, we might want to make this new behavior the default
at some point, and deprecate the previous one. But this is out of scope for this RFC.

Remove globals usage

As described above, when opting into no-globals mode, an Ember app built using Ember-CLI, including optionally
ember-auto-import and/or Embroider, should not read from and leak any globals, with the exceptions outlined above.

The following identifies current global usage, and outlines possible ways to remove their usage, or at least prevent the
chance of conflicts. Note that this RFC does not aim to prescribe a specific implementation, as besides the visible
behavior of not relying on globals there is no public API introduced here, so the actual implementation remains the task
of any implementor of this RFC. The implementation outlines are merely intended to demonstrate the feasibility of the
proposed changes.

AMD-like module system

The global functions/namespaces define and require (and its aliases requireModule, require, requirejs) are set
as globals and provided by the 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()
calls in vendor.js (for addon code) or app.js (for app code) would call an undefined function. So it is effectively a
mandatory dependency currently.

Despite that, Ember comes with its own (stripped down) loader library,
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), so dead code effectively.

With that, a typical build of vendor.js will look basically like this:

vendor.js
// EmberENV code, ommited here, see following chapter

var loader, define, requireModule, require, requirejs;

(function(global) {
  
  // loader.js IIFE defing the globals above
  // see https://github.com/ember-cli/loader.js/blob/master/lib/loader/loader.js

})(this);

(function() {
  var define, require;
  
  (function () {
    // Ember's own internal loader code
  
    // ...
    // globalObj = globalThis w/ polyfill
    if (typeof globalObj.define === 'function' && typeof globalObj.require === 'function') {
      define = globalObj.define;
      require = globalObj.require;
  
      return;
    }
    
    // Following are Ember's own (internal) implementation of define and require
    // However due to the early return above and the fact that loader.js always prepends its code, thus assigns
    // the define and require globals, this code is never reached! (when the loader.js addon is present!)
    
  })();
  define("@ember/-internals/bootstrap/index", /*...*/);
  // ... 
  // all of Ember's own modules
  // ...
  require('@ember/-internals/bootstrap')
})();

define('@ember-data/adapter/-private', /* ... */);
// ... 
// all modules from Ember addons
// ...

And this is what a typical compilation of app.js will look like:

app.js
define("my-app/app", /* ... */);
// ... 
// all app modules
// ...

As you can see it relies on a globally defined define, that has been declared in the first line of vendor.js, and set
by the code of loader.js
.

As to how to prevent the global usage while retaining compatibility of any code that uses those functions as globals,
we can make the build system emit the existing code wrapped in a IIFE,
that avoids polluting the global namespace, while still any calls of define() or require() will see the same functions,
they just do not come from the global namespace anymore.

This works for vendor.js, where these functions are defined, but not for app.js which needs access to the same functions
as defined in vendor.js. That's why we still need some usage of globals, but as outlined in the previous chapter, we
must make sure they are scoped to the current app, so they pose no risk of conflicts.

So for vendor.js this is what the basic change could look like:

vendor.js
+ (function(){
var loader, define, requireModule, require, requirejs;

(function(global) {
  
  // loader.js IIFE defing the globals above
  // see https://github.com/ember-cli/loader.js/blob/master/lib/loader/loader.js

})(this);
+ globalThis.__my-app_12345_define = define;
+ globalThis.__my-app_12345_require = require;
- (function() {
-   var define, require;
-   
-   (function () {
-     // Ember's own internal loader code
-   
-     // ...
-     // globalObj = globalThis w/ polyfill
-     if (typeof globalObj.define === 'function' && typeof globalObj.require === 'function') {
-       define = globalObj.define;
-       require = globalObj.require;
-   
-       return;
-     }
-     
-     // Following are Ember's own (internal) implementation of define and require
-     // However due to the early return above and the fact that loader.js always prepends its code, thus assigns
-     // the define and require globals, this code is never reached! (when the loader.js addon is present!)
-     
-   })();
  define("@ember/-internals/bootstrap/index", /*...*/);
  // ... 
  // all of Ember's own modules
  // ...
  require('@ember/-internals/bootstrap')
- })();

define('@ember-data/adapter/-private', /* ... */);
// ... 
// all modules from Ember addons
// ...
+ })();

Note that we were able to drop all of Ember's internal module code here!

And this for app.js:

app.js
+ (function() {
+ var define = globalThis.__my-app_12345_define;
+ var require = globalThis.__my-app_12345_require;
define("my-app/app", /* ... */);
// ... 
// all app modules
// ...
+ })();

By wrapping the code in the IIFE and defining define and require within it, we are basically hiding any eventually defined
globals (e.g. 3rd party RequireJS), while letting all existing code use our implementation as if it were globally defined.

Note that this does only work when code uses these functions as implicit globals. When explicitly referring to the global
namespace like window.require() or globalThis.require(), then this will inevitably fail.

Besides these two bundles, which make up "classic" Ember apps, we must also take into account the additional bundles
("chunks") that ember-auto-import and Embroider would emit.

As ember-auto-import and Embroider emit similar webpack-compiled chunks, we will only highlight the required changes
in the case of ember-auto-import here.

ember-auto-import emits a chunk.app.xxx.js chunk, which includes the boundary between Ember's AMD-like modules and
whatever Webpack emits for the imported modules it handled, so this is basically the glue between these two worlds. It typically
looks something like this (cleaned up and simplified):

chunk.app.xxx.js
var __ember_auto_import__;
(() => {
  var __webpack_modules__ = ({

    "../../../../private/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-4171121iPrUeBlpJ8/cache-307-webpack_bundler_ember_auto_import_webpack/app.js":
      ((module, __unused_webpack_exports, __webpack_require__) => {
        module.exports = (function() {
          var d = _eai_d;
          var r = _eai_r;
          window.emberAutoImportDynamic = function(specifier) {
            if (arguments.length === 1) {
              return r("_eai_dyn_" + specifier);
            } else {
              return r("_eai_dynt_" + specifier)(Array.prototype.slice.call(arguments, 1));
            }
          };
          window.emberAutoImportSync = function(specifier) {return r("_eai_sync_" + specifier)(Array.prototype.slice.call(arguments, 1));};
          d("lodash-es/find", [], function() { return __webpack_require__(/*! lodash-es/find */ "./node_modules/lodash-es/find.js");});
          d("lodash-es/startsWith", [], function() {return __webpack_require__(/*! lodash-es/startsWith */ "./node_modules/lodash-es/startsWith.js");});
          d("_eai_dyn_lodash-es/concat", [], function() {return __webpack_require__.e(/*! import() */ "node_modules_lodash-es_concat_js").then(__webpack_require__.bind(__webpack_require__, /*! lodash-es/concat */ "./node_modules/lodash-es/concat.js"));});
        })();
      }),

    "../../../../private/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-4171121iPrUeBlpJ8/cache-307-webpack_bundler_ember_auto_import_webpack/l.js":
      (function(module, exports) {
        window._eai_r = require;
        window._eai_d = define;
      })
  });
  // ... webpack internals 
})();

Note that ember-auto-import itself creates new globals _eai_* here, that we must prevent. And it takes Ember's require
and define from the global namespace. A "fixed" version could look like this:

chunk.app.xxx.js
var __ember_auto_import__;
(() => {
  var __webpack_modules__ = ({

    "../../../../private/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-4171121iPrUeBlpJ8/cache-307-webpack_bundler_ember_auto_import_webpack/app.js":
      ((module, __unused_webpack_exports, __webpack_require__) => {
        module.exports = (function() {
-          var d = _eai_d;
+          var d = globalThis.__ember_12345_define;
-          var r = _eai_r;
+          var r = globalThis.__ember_12345_require;
          window.emberAutoImportDynamic = function(specifier) {
            if (arguments.length === 1) {
              return r("_eai_dyn_" + specifier);
            } else {
              return r("_eai_dynt_" + specifier)(Array.prototype.slice.call(arguments, 1));
            }
          };
          window.emberAutoImportSync = function(specifier) {return r("_eai_sync_" + specifier)(Array.prototype.slice.call(arguments, 1));};
          d("lodash-es/find", [], function() { return __webpack_require__(/*! lodash-es/find */ "./node_modules/lodash-es/find.js");});
          d("lodash-es/startsWith", [], function() {return __webpack_require__(/*! lodash-es/startsWith */ "./node_modules/lodash-es/startsWith.js");});
          d("_eai_dyn_lodash-es/concat", [], function() {return __webpack_require__.e(/*! import() */ "node_modules_lodash-es_concat_js").then(__webpack_require__.bind(__webpack_require__, /*! lodash-es/concat */ "./node_modules/lodash-es/concat.js"));});
        })();
      }),

-    "../../../../private/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-4171121iPrUeBlpJ8/cache-307-webpack_bundler_ember_auto_import_webpack/l.js":
-      (function(module, exports) {
-        window._eai_r = require;
-        window._eai_d = define;
-      })
  });
  // ... webpack internals 
})();

Note that ember-auto-import needs knowledge about the obfuscated names of the (still globally defined) functions (e.g.
__ember_12345_define in this example) here. This should be exposed to privileged addons like ember-auto-import or
Embroider somehow, but as this is a strictly private API, it is not detailed here.

EmberENV

Ember reads environment variables from a EmberENV hash to enable/disable certain runtime features. Usually this is set
from the app's config/environment.js and @ember/optional-features, and compiled into the top of vendor.js:

vendor.js
window.EmberENV = (function(EmberENV, extra) {
  for (var key in extra) {
    EmberENV[key] = extra[key];
  }

  return EmberENV;
})(window.EmberENV || {}, {"FEATURES":{},"EXTEND_PROTOTYPES":{"Date":false},"_APPLICATION_TEMPLATE_WRAPPER":false,"_DEFAULT_ASYNC_OBSERVERS":true,"_JQUERY_INTEGRATION":false,"_TEMPLATE_ONLY_GLIMMER_COMPONENTS":true});

But as you can see there, you can also inject those by setting a globally defined window.EmberENV.

In no-globals mode though, the latter behavior will be disabled. Only the variables built into vendor.js are taken
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).

Deprecate direct usage of AMD-like module APIs

TO DO

When opting into no-globals, it seems reasonable to also deprecate direct usage of define and require (the latter is not uncommon)
in user-land code. We can still make the code emitted by the build system not cause any deprecations, but add deprecations
at least for require in user's code. I hope for any need of dynamic module usage, we can provide a transition path
using embroider/macros (using e.g. dependencySatisfies() and importSync()). This would make the AMD module system
become a private implementation detail, thus easing possible future changes of the way we package apps!?

Should this be part of this RFC? It could be a separate, on the other hand it is tied to the new no-globals mode proposed here.

How we teach this

What names and terminology work best for these concepts and why? How is this
idea best presented? As a continuation of existing Ember patterns, or as a
wholly new one?

Would the acceptance of this proposal mean the Ember guides must be
re-organized or altered? Does it change how Ember is taught to new users
at any level?

How should this feature be introduced and taught to existing Ember
users?

TO DO

  • explain how to opt in
  • not much to teach otherwise, this affects only build tooling

Drawbacks

Why should we not do this? Please consider the impact on teaching Ember,
on the integration of this feature with other existing and planned features,
on the impact of the API churn on existing apps, etc.

There are tradeoffs to choosing any path, please attempt to identify them here.

TO DO

Alternatives

What other designs have been considered? What is the impact of not doing this?

This section could also include prior art, that is, how other frameworks in the same domain have solved this problem.

TO DO

Unresolved questions

  • Do we need all the APIs of current loader.js? Or can we ship our own stripped version of loader.js, e.g. without noConflict(), .unsee(), .alias, what else? Remember that we are opting into a new mode, so we can drop things and define our new standard, that is basically a strictly private API now, right?
  • What about Ember.__loader? Still needed? How is it even supposed to be used, with the Ember global gone? If you are supposed to require('ember'), you already used the global require, so what is Ember.__loader.require for?
  • relevant PR: Refactor the internal Ember loader to use the standard Ember CLI loader ember.js#19390. What does "Loader code is still included for Node support. If define and require are not already defined, then a backup shim is used instead." mean? Why would they not be defined for node?
@scottmessinger
Copy link
Contributor

This is interesting! I have a question (which is probably pretty naive) -- why does Ember need to continue to use an AMD-style loader? Why doesn't Ember move to export ES modules by default and provide the existing AMD-style loader as an optional build target? Is this because of browser support issues or limitations in the framework or something else?

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Dec 16, 2021

Why doesn't Ember move to export ES modules by default and provide the existing AMD-style loader as an optional build target?

I really want ESM as a target format. It would help all my projects immensely (and enable skypack usage), and give a pretty good build-speed boost, as there would be no conversion to AMD (at least for dev-time). ESM for production builds would be technically challenging, cause you'd need to still create a concattenated file of uniquely named things, and you lose module boundaries... which could be dangerous in some cases (side-effects, name collisions, etc)

for REPLs especially, I really want to be able to do this:
image
embroider-build/embroider#984 (comment)

@bendemboski
Copy link
Collaborator

@simonihmig I'm very much in support of something along these lines, and I'd like to contribute another use case, which is using ember-electron to run an Ember app in an Electron window with node integration enabled. In this case, the global require() is Node.js's require() initially, and then Ember overwrites it with the loader.js require(), which is obviously a problem for running Node.js code.

To work around this, ember-electron:

  • injects a script into index.html before the app and vendor scripts that renames window.require (the Node.js require) to window.requireNode
  • injects a script into vendor.js (after the Ember code) that replaces window.require (which is now the loader.js require) with a custom require function that tries requiring via both module loaders (Node.js and loader.js)

It works, but it's not pretty, clearly has cases where it would fail (Ember-bundled modules with the same name as Node.js modules but different functionality), and I would love it if something like this RFC removed the need for this hackery.

@lifeart
Copy link

lifeart commented Dec 16, 2021

One more case to highlight here - we have to figure out how to share namespace visibility between engines (it's considered as a "standalone" apps, but should be aware of parent app and it's modules)

@simonihmig
Copy link
Contributor Author

I'd like to contribute another use case, which is using ember-electron

Oh, that's an excellent addition! I didn't know about that, will add it when preparing the real RFC! Thanks @bendemboski

we have to figure out how to share namespace visibility between engines (it's considered as a "standalone" apps, but should be aware of parent app and it's modules)

Oh, yes, good point! (not an engines user, so I tend to forget about them). Thanks @lifeart

@simonihmig
Copy link
Contributor Author

I have a question (which is probably pretty naive) -- why does Ember need to continue to use an AMD-style loader?

Actually, this is a very good question! I am by far not the best to answer that, but I'll try by saying it is probably a combination of...

  • legacy: usage of require() is wide-spread, not only in core parts like ember-resolver, but also in user-land code. That's also why deprecating that would make sense for me, so we can lay the path for getting rid of that AMD stuff.
  • dynamic usage: many parts of an Ember app are not imported statically, rather they go through Ember's Dependency Injection and resolver. That currently applies to all these things like controllers, routes, services, models and (still) components. And you can do wild things, like registering new classes with the DI container at runtime, so AFAICT you also need some run-time module system for that. AFAIK if things were fully static, bundlers like rollup could just run over it and compile it to a bundle that contains no run-time modules anymore, just something like a blob of code (wrapped in IIFE or something). But we have so much implicitness in Ember apps, that's basically one of the main reasons we need Embroider for.

@simonihmig
Copy link
Contributor Author

@bendemboski I was iterating a bit on my RFC draft, adding your use case, when I realized that the proposal I had in mind (wrapping code in an IIFE to prevent polluting the global namespace, while still allowing to use require() as if it were a global) is actually not addressing it. Even if require() is not global anymore, within your app's bundles you still only see one ambiguous require(), which has to cover both the AMD-style use of Ember's modules as well as node.js modules. Thus these workarounds you mentioned are still needed. Technically you could use require() for Ember modules, and globalThis.require() for node modules, but that doesn't work for all these instances of require() in node.js based 3rd party npm modules you might want to use...

I think the only way to address this is to actually get totally rid of require() as part of Ember's module system, at least as far as user-land code is concerned. Basically dropping AMD as the (semi-)public API we currently have. Which would be good eventually IMO, but is a significantly larger step than I had in mind for now.

@bendemboski
Copy link
Collaborator

@simonihmig I may need to clarify something. The require overloading workaround we have in place currently addresses two use cases:

  1. Bundled code using require() to load Node.js modules. We recommend against this, and recommend that users instead use requireNode() (which, as stated above, is just Node.js' require copied to a different global symbol) in bundled code, but it does currently work.
  2. Code that is loaded via Node.js' module loader that then itself uses require() to load additional code.

I'm pretty sure (2) would be unaffected by the IIFE setup, right? Because code loaded via Node.js' module loader would be unaffected by the IIFE, and references to the global require symbol would point to whatever that actually is at runtime, not the IIFE-scoped symbol, and with the current ember-electron setup the runtime global require would be Node.js' require (that's what happens if ember-electron detects that nobody wrote to the global require symbol during the loading of the Ember-bundled code).

So as far as ember-electron is concerned, we would no longer need a workaround/wrapper (ignoring backwards compatibility) for case (2) because the global require symbol would be Node.js' require because Ember would no longer overwrite it, correct?

So that leaves (1), which is a stickier problem because there is inherent ambiguity -- if bundled source code says require, then as long as Ember supports referencing its require as a global symbol in source code we can't reliably know if the user's intent is to call Node.js' require or Ember's. So here I agree with you that until/unless we do the out-of-scope-of-this-RFC thing of pulling support for Ember's global require in source code, ember-electron will still need to do something to address this situation. However, with (2) addressed by this RFC, we should have the option of removing the require overload function and (with a major version bump or opt-in) requiring users to use requireNode() from any bundled code when loading Node.js modules. And then I guess allow them to switch back to require() if/when we pull the global require from Ember's API.

So I think this RFC does help ember-electron quite a bit. The only place it doesn't help is in the place that's un-helpable because given the current conflicting APIs, we can't determine the user's intent when they write require in to-be-bundled source code.

However, I think there's another consideration. If this RFC were implemented today with no changes to ember-electron, cases where bundled code invokes Node.js' require() via the global symbol require would stop working (I'm pretty confident all other cases would continue working) because at runtime that would no longer invoke the patched window.require(), but would instead call directly into the AMD require() via the IIFE-scoped symbol. So, unless there's some way for ember-electron to hook into and/or wrap that AMD require(), there would be no way to prevent this from being a breaking change for ember-electron users that invoke require() from bundled code to load node modules. In that case users would have to switch all of their invocations to use requireNode() before being able to opt in, which I think is probably acceptable even if not ideal.

@simonihmig
Copy link
Contributor Author

@bendemboski thanks a lot for your extensive feedback!

I'm pretty sure (2) would be unaffected by the IIFE setup, right?

I was indeed worried about your case 2) here, but you are right, the IIFE wouldn't change anything. I didn't think this through correctly. So yeah, that's good actually!

we should have the option of removing the require overload function and (with a major version bump or opt-in) requiring users to use requireNode() from any bundled code when loading Node.js modules.

Yes, I agree that would be the cleaner solution than the overloaded require(). Similar actually to what we have with FastBoot.require(). And given that is already the recommended way in electron as I learned today, it seems reasonable to make this mandatory in the future IMHO...

there would be no way to prevent this from being a breaking change for ember-electron users that invoke require() from bundled code to load node modules

Yes. But that's why we wrap that new behavior in an optional feature, as there are edge cases like this one where this is a breaking change. So with the RFC implemented, the message for electron users would be that it is recommended (and the old setup not using no-globals probably deprecated at some point) to refactor all existing source code to use requireNode() where node modules are loaded, and then opt into no-globals. In which case ember-electron could detect that enabled feature and stop doing its current hackery. Do you agree?

@simonihmig
Copy link
Contributor Author

Closing this in favor of the real thing: #784. Thanks for all your feedback so far!

@bendemboski
Copy link
Collaborator

@simonihmig yes, I agree. Assuming nothing is changed that breaks ember-electron's existing workarounds when not opting in to no-globals, this is a step forward for ember-electron because users can choose to leave things as-is (subject to whatever deprecation/removal timeframe), or to ensure they use requireNode() and then opt in and get more stable behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants