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

Version 3.10.1 regression: use of const in strict mode, causing critical errors in e.g. Safari 9 #18084

Open
samcic opened this issue Jun 9, 2019 · 12 comments

Comments

@samcic
Copy link

commented Jun 9, 2019

In ember.min.js version 3.10.1 there are various usages of the const keyword in use strict mode:

  • @ember/-internals/glimmer, example: const e=L(["template-compiler:main"]
  • @ember/application/lib/application, example: const e=function(e,t){t...
  • @ember/engine/index, example: const e=v(["-bucket-cache:main"])
  • @ember/engine/instance, example: const e=u(["template-compiler:main"])

These cause syntax errors in Safari 9 (e.g. on iPhone 6S) and the Ember app fails to start. The error is:

SyntaxError: Use of const in strict mode

Ember 3.9.1 (the previous version) didn't have this problem.

Repro:

npm install -g ember-cli@3.10.1
ember new test-app
yarn install
(edit config/targets.js to include Safari >= 9)
ember build -prod
(inspect dist/vendor.js and see the above-mentioned const occurrences)

Is this a bug, or am I missing something with respect to the build process?

@rwjblue

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

Hmm, this is indeed surprising. If you add IE11 to the config/targets.js do you still see the consts? I'm guessing that our detection of whether or not we should use the "legacy" build or not is deciding not to use legacy when it should.

My best guess is that this as to do with #17859, as that is one of the only changes in the build pipeline (specifically WRT selecting legacy or modern builds).

@samcic

This comment has been minimized.

Copy link
Author

commented Jun 9, 2019

Yes, the consts are still there if IE11 is included in the targets. Is there a way to force the legacy build mode?

@samcic

This comment has been minimized.

Copy link
Author

commented Jun 9, 2019

One thing I have noticed in the build process is the message:

Browserslist: caniuse-lite is outdated. Please run next command yarn upgrade caniuse-lite browserslist

I'm guessing that this is a dependency of one of the ember-related packages and it's something that we're not responsible for updating ourselves. I wanted to mention it regardless though.

@austinezell

This comment has been minimized.

Copy link

commented Jun 25, 2019

I am currently experiencing this issue as well. The only additional thing I notice that might be worth pointing out is that the application specific code is still being correctly transpiled to legacy; in the above repro steps, the test-app.js file generated successfully strips out all let/const or other non supported features based on what is in the targets.js. It is only the vendor.js file that appears to fail.

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@austinezell this is because we actually do not compile any of Ember's code using your targets. At the moment Ember ships with two builds, the legacy build with everything transpiled, and the modern build targeted at the most recent browsers. We use feature detection based on your presets to pick which one to use, but this is the second time that something has broken because of it.

I think at this point we really need to work toward compiling Ember just-in-time using the host application's build targets. It may end up being a bit of a hacky solution, but it'll prevent these kinds of issues from cropping up.

@austinezell

This comment has been minimized.

Copy link

commented Jun 26, 2019

I just tried to npm link this package and force my app to pull in the "legacy" version of ember. I ended up just verifying that my application is in fact already pulling in the legacy version.

The issue is that the legacy currently has 15 instances of const. Cloning this repo and running npm build then checking the dist/legacy/ folder verifies this. It does seem that @rwjblue's assumption is correct and that this was caused by #17859 . Re-adding the line ['@babel/transform-template-literals', { loose: true }], into the to-es5.js module and re-running npm build successfully removed all instances of const from the legacy build.

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Ah, interesting, I thought he meant it was the same type of issue, not that exact fix. This complicates things further, because we had to remove that check as it was causing users who aren't supporting legacy browsers to fail. I think we really need to move things to build dynamically, I'll try to get on this soon.

@BnitoBzh

This comment has been minimized.

Copy link

commented Jul 3, 2019

Is there anything new about this problem?

@lifeart

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

one more related issue: #17809 (comment)

@BnitoBzh

This comment has been minimized.

Copy link

commented Jul 16, 2019

There is nobody to fix this issue ? or revert/update this merged PR #17859 ?

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

I’m currently working on the fix for this. It is not easy, it means some pretty major changes to Ember’s build and testing setup, so that it dynamically compiles based on the consuming application’s Babel settings and targets.

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

This will be fixed by #18208, assuming we can get it landed. I'm not sure how possible it'll be to backport, I've labeled it as LTS but I'm very doubtful we'd be able to get it back to v3.4. I think v3.8 would be possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.