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

Drop FastBoot.require for 3rd dependencies? #251

Open
bobisjan opened this issue Nov 3, 2019 · 5 comments
Open

Drop FastBoot.require for 3rd dependencies? #251

bobisjan opened this issue Nov 3, 2019 · 5 comments
Labels

Comments

@bobisjan
Copy link
Contributor

bobisjan commented Nov 3, 2019

Hello,

according to the upcoming major release, would it make sense to drop custom FastBoot import system and use what JavaScript has by spec?

Some benefits could be

  • following the spec,
  • simplifying FastBoot build process.

3rd dependencies

For importing external modules from 3rd packages the ember-auto-import could be used.

  1. Add dependencies as usual npm install --save-dev ldclient-node ldclient-js.

  2. Use dynamic import to conditionally import modules in FastBoot / browser.

// simplified example

let ldclient;

if (typeof FastBoot !== 'undefined') {
  ldclient = await import('ldclient-node');
} else {
  ldclient = await import('ldclient-js');
}

Node.js built-in modules

FastBoot.require will be still used for importing Node.js builtins at runtime, however it could be marked "private", because only ember-auto-import should use it at compile time, add-ons/applications will use native JavaScript.

// input
import os from 'os';
const os = await import('os');

// output
const os = FastBoot.require('os'); 
const os = await FastBoot.require('os');

I'm not sure if I'm missing something that could prevent from dropping FastBoot.require now.

Thanks

@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2019

I'm definitely in favor of doing things to simplify and cleanup how to work with FastBoot, and this does seem like a logical thing to reevaluate. There are a few things that I don't fully understand how you would do in the new system:

  • Since inside an Ember application there is already a global variable named require (defined here), I don't see how you'd gain access to the Node-land require without having it exposed in some way (e.g. FastBoot.require).
  • If we used dynamic import statements for this instead:
    • How do we absorb the asynchrony introduced? (not all locations that you would need to FastBoot.require today allow for async absorption)
    • Do we need to make ember-auto-import (and embroider) aware of this specific pattern, so that it knows that the shim it uses to implement dynamic requires should use node-land vs in-app systems?

If we do move forward with this proposal, I think that we would need a decent length deprecation cycle for FastBoot.require. So this probably wouldn't be something we can straight up remove in v3.0.0 (which gives us a bit of time to review/analyze the best path forward).

@bobisjan
Copy link
Contributor Author

bobisjan commented Nov 3, 2019

Since inside an Ember application there is already a global variable named require (defined here), I don't see how you'd gain access to the Node-land require without having it exposed in some way (e.g. FastBoot.require).

If I understand ember-auto-import correctly, the FastBoot.require is not used for importing from 3rd packages in FastBoot, because add-on handles this on its own via webpack.

The problem is with Node.js built-in modules, for that the FastBoot.require should be still required if using buildSandboxGlobals() is not ideal. Another option might be to allow dynamic imports of Node.js builtins in FastBoot - ember-auto-import will probably still be using internally FastBoot.require, but that would be abstracted away from end-user.

How do we absorb the asynchrony introduced? (not all locations that you would need to FastBoot.require today allow for async absorption)

You mean that await import('...') at top level is not supported today in Ember? Maybe some temporary dynamic but static importSync() provided by ember-auto-import can be introduced to proceed before top level await is supported (this will not use FastBoot.require)?

Do we need to make ember-auto-import (and embroider) aware of this specific pattern, so that it knows that the shim it uses to implement dynamic requires should use node-land vs in-app systems?

I'm not sure if I understand correctly, but the ember-auto-import seems to already know how to dynamically import 3rd packages both when it is running in browser or in FastBoot.


If we do move forward with this proposal, I think that we would need a decent length deprecation cycle for FastBoot.require. So this probably wouldn't be something we can straight up remove in v3.0.0 (which gives us a bit of time to review/analyze the best path forward).

That makes totally sense 👍.

@rwjblue
Copy link
Member

rwjblue commented Nov 3, 2019

If I understand ember-auto-import correctly, the FastBoot.require is not used for importing from 3rd packages in FastBoot, because add-on handles this on its own via webpack.

This is definitely true, but today the distinction between import and not means that we never accidentally ship things inside the runtime assets that are from FastBoot.require. Changing to use import here means that we need to make sure that those imports aren’t bundled into the main (non-“FastBoot only”) assets.

You mean that await import('...') at top level is not supported today in Ember?

No, not specifically. I mean that using import() should never return the imported thing synchronously. It returns a promise, and when that promise resolved the imported thing is returned. Not all codepaths that exist today allow absorbing a promise (e.g. service methods, any objects init, etc). I don’t think this is fundamentally fatal, just requires some thought.

I'm not sure if I understand correctly, but the ember-auto-import seems to already know how to dynamically import 3rd packages both when it is running in browser or in FastBoot.

That’s great! My point was more that we need to make sure that we don’t accidentally bloat our runtime assets with SSR specific code.

@bobisjan
Copy link
Contributor Author

bobisjan commented Apr 12, 2020

Based on the exploration I've made embroider-build/ember-auto-import#266 with @ef4's help, I'm going to update the proposal to "drop" FastBoot.require for 3rd dependencies, and use it only for Node.js builtins.

There are already add-ons, like ember-useragent, which use ember-auto-import instead of FastBoot.require.

There are also some add-ons, like ember-fetch, which can not use EAI until Embroider I assume, until that this will be just a proposal.

However, because ember-fetch is the last add-on in our stack, which requires npm install on build/deploy, I've proposed an interim change ember-cli/ember-fetch#493. This will allow us to remove npm install from ember-fastboot/fastboot-s3-downloader#18 and avoid issues like #221.

@bobisjan bobisjan changed the title Drop FastBoot.require by default? Drop FastBoot.require for 3rd dependencies? Apr 12, 2020
@ef4
Copy link
Contributor

ef4 commented Apr 13, 2020

A few updates to the earlier questions in this thread based on discussion around the embroider v2 addon format RFC:

How do we absorb the asynchrony introduced?

This is why the importSync macro is in that RFC. It gives us one declarative way to handle this problem wherever it comes up. Also, as an optional extension we could choose to allow top-level await inside macroCondition-guarded conditionals, because that's much easier to implement that allowing top-level await everywhere.

Since inside an Ember application there is already a global variable named require (defined here), I don't see how you'd gain access to the Node-land require without having it exposed in some way (e.g. FastBoot.require).

The point would be eliminating the distinction. There would no longer be any way to access node's require. Everything that's allowed to be used in fastboot would be packaged into the build already, so that Ember's require knows how to hand it back to you.

Do we need to make ember-auto-import (and embroider) aware of this specific pattern, so that it knows that the shim it uses to implement dynamic requires should use node-land vs in-app systems?

No. Realize that for both auto-import and embroider the distinction already doesn't exist. Trying to maintain the distinction is huge source of pain and complexity.

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

No branches or pull requests

4 participants