[Bug] Optional chaining not transpiled for node/FastBoot

[simonihmig]

🐞 Describe the Bug

The Ember code base contains instances of optional chaining, in this case it' about this line. Although targets have been properly defined (node: 'current'), when running a fresh new app with FastBoot in node 10, this does not get transpiled as expected, and FastBoot throws.

🔬 Minimal Reproduction

Here is a reproduction: https://github.com/simonihmig/ember-transpilation-reproduction. This is basically a fresh new 3.24 app, with ember-cli-fastboot installed and targets.js updated for node.

Make sure you are using node 10 (pinned for volta), then run yarn start to see the FastBoot error

😕 Actual Behavior

Optional chaining should get transpiled for node 10, which does not support it. But it does not, and node 10 throws this when running the app in FastBoot:

/var/folders/q3/yzggf_l965zbzps_bk8xr37c0000gn/T/broccoli-12464N2d65poMnE2Z/out-170-append_ember_auto_import_analyzer/assets/vendor.js:26317
      this.forEach(item => ret.push(item[methodName]?.(...args)));
                                                     ^

SyntaxError: Unexpected token .
    at new Script (vm.js:83:7)
    at VMSandbox.eval (/Users/simonihmig/Projects/test-addon/node_modules/fastboot/src/vm-sandbox.js:13:22)
    at /Users/simonihmig/Projects/test-addon/node_modules/fastboot/src/ember-app.js:190:15
    at Array.forEach (<anonymous>)
    at EmberApp.loadAppFiles (/Users/simonihmig/Projects/test-addon/node_modules/fastboot/src/ember-app.js:187:21)
    at EmberApp.retrieveSandboxedApp (/Users/simonihmig/Projects/test-addon/node_modules/fastboot/src/ember-app.js:235:10)
    at new EmberApp (/Users/simonihmig/Projects/test-addon/node_modules/fastboot/src/ember-app.js:61:21)
    at FastBoot._buildEmberApp (/Users/simonihmig/Projects/test-addon/node_modules/fastboot/src/index.js:114:17)
    at new FastBoot (/Users/simonihmig/Projects/test-addon/node_modules/fastboot/src/index.js:52:10)
    at app.use (/Users/simonihmig/Projects/test-addon/node_modules/ember-cli-fastboot/index.js:335:29)

🤔 Expected Behavior

Correct transpilation for node 10.

🌍 Environment

• Ember: 3.24
• Node.js/npm: 10

➕ Additional Context

If you run CI=true yarn start instead - enabling transpilation for IE11 - the optional chaining does get transpiled as expected!

Opening this issue here, as the default DX of a fresh Ember app w/ FastBoot is affected. But the root cause could of course lie elsewhere. Maybe @babel/preset-env is somehow now working correctly with the node config option?

[pzuraq]

@simonihmig when I run npx browserslist in a package with:

"browserslist": [
    "current node"
  ]

In package.json I get:

node 14.15.1

I believe the correct selector for all maintained node versions is:

"browserslist": [
    "maintained node versions"
  ],

I'm also not sure about how this translates to the targets.js file exactly, in the sense that, I'm pretty sure you could add this to the browsers array, but less sure about how the node property you've set gets read/included in the targets or if it does at all. But either way, I don't think current refers to lowest supported LTS. Can you confirm?

[simonihmig]

I'm also not sure about how this translates to the targets.js file exactly

Well, whether that's right or not, but I assume that targets.js is basically following what @babel/preset-env expects as targets, isn't it?

If you want to compile against the current node version, you can specify "node": true or "node": "current", which would be the same as "node": process.versions.node.

I read their docs in a way that you can have a browsers array, but also a node property (sibling to browsers, which when set to true or 'current' would transpile for your version of node, not the latest.

See https://babeljs.io/docs/en/babel-preset-env#targetsnode.

Fwiw, there was a (merged) PR for ember-cli-fastboot by @mansona, that adds this setting as part of its blueprint. So I think that's the expected way to do it!?

[pzuraq]

mmk, dug in, the issue is that we use a pre-built version of Ember only for development builds. The reason we do this is so that compiling Ember does not affect the DX as much - it's a decently long compilation process, and when we started doing dynamic compilation we decided to use a prebuild version to avoid a regression.

The issue is that the logic currently only takes into account the browsers property on targets. It compares to see if you have the exact same browsers as what we pre-compiled to.

From this perspective, the node: "current" setting that @mansona added is a bit problematic, since not everyone uses Volta. Unlike the browsers, which we have a known ahead-of-time compile target for, node can shift depending on the user's machine. So, we'll need to match Babel's semantics for this field, both for our ahead-of-time compile target and matching it when we compile on the user's machine.

To be honest, the double-layered setting that @babel/preset-env has seems confusing. I was able to fix it and get it to compile correctly by doing:

module.exports = [
  ...browsers,
  'maintained node versions'
];

I think this would be a better set of defaults in the future, that way we would just be using browserslist. Definitely would need to figure out a transition path though.

[pzuraq]

To fix this bug as is, I think the steps are:

• Add current node to the browserslist that Ember uses to precompile the prebuild
• Update the logic on the ember addon main that that chooses whether to build ember-source or use the prebuild to:
  1. Check that browsers and node are the only values that are set on targets
  2. Check that browsers is equal to the set of prebuilt browsers (the current behavior)
  3. Check that node is equal to "current"
  4. Load the "current" version of Node according to browserslist, and compare it to the running version of Node on the user's machine. If the running version is a major that is lower than the browserslist "current", then we should compile. If it is the same major, then we can safely use the prebuild.

I don't think I'll have time to implement this in the near future, but am happy to help advise and review PRs!

[simonihmig]

To be honest, the double-layered setting that @babel/preset-env has seems confusing. I was able to fix it and get it to compile correctly by doing:

I can confirm this fixes the problem! And yes, that Babel config is quite confusing: added 'maintained node versions' to the existing array as part of the browsers key of the exported object does not work, but exporting a plain array with browsers including 'maintained node versions' as shown in your snippet does. 🤯

Thanks for the super quick investigation! 🙏

[mansona]

Hey folks 👋 I am here to help get this fixed ASAP if y'all don't mind giving me a bit of guidance. This is causing a bit of pain for the whole Empress ecosystem since this line was added to Ember https://github.com/emberjs/ember.js/blob/v3.24.0/packages/%40ember/-internals/runtime/lib/mixins/array.js#L1350 and I can confirm what you say @pzuraq , this is only affecting development.

Regarding To be honest, the double-layered setting that @babel/preset-env has seems confusing: defining the browsers the way that we do now is actually deprecated. See https://babeljs.io/docs/en/babel-preset-env#targetsbrowsers

Note: this will be removed in later version in favor of just setting "targets" to a query directly.

I know it's a bigger question to move over to using the recommended structure but that explains why it seems so complicated. It might also complicate the recommended solution to this if we want to follow the "proper" way of doing this 🤔 I'll put together my best guess at a PR now and we can use that as a discussion point 👍

[mansona]

Also I think you might have a slight misunderstanding of what node: "current" means... It's not what is considered current in the wider community, it is what is currently on the user's machine:

If you want to compile against the current node version, you can specify "node": true or "node": "current", which would be the same as "node": process.versions.node.

I think if we are pre-building using CI with is set to the oldest LTS (Node 10) then I think we're good 👍

[mansona]

It turns out that actually trying to effectively pre-build this wasn't going to be easy 🤔 especially with the issues around what "node: current" actually means. I have confirmed that my PR works and to be honest I don't really see that much of an impact on build time so if you're happy with it @pzuraq then I think we can go ahead with it 🎉

[rwjblue]

Fixed by #19397

[simonihmig]

I know it's a bigger question to move over to using the recommended structure but that explains why it seems so complicated. It might also complicate the recommended solution to this if we want to follow the "proper" way of doing this 🤔 I'll put together my best guess at a PR now and we can use that as a discussion point 👍

To add to this, while defining the targets in anyway supported by @babel/preset-env - like returning a plain array without browsers as suggested before - works for Babel, other addons do expect an object with browsers, which is also what the (quite old) original RFC specified.

I just realized that I caused a regression in the deployment setup by this, as ember-cli-deploy-compress expects browsers. So I was very lucky to realize just by coincidence that this caused our assets to not get compressed using brotli anymore (just plain gzip), would be very easy to totally miss this... 🙈