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

[BUGFIX lts] Fix leftover const expressions in legacy builds #18576

Merged
merged 1 commit into from
Nov 26, 2019

Conversation

kanongil
Copy link
Contributor

This fixes #18575. Feel free to backport to 3.8.

The transform-block-scoping plugin is needed since the transform-template-literals addon injects a number of _templateObject() functions into the code, within which is a const assignment.

@rwjblue
Copy link
Member

rwjblue commented Nov 25, 2019

Feel free to backport to 3.8.

Following our normal LTS policy, 3.8 is receiving security fixes only at this point.

@kanongil
Copy link
Contributor Author

kanongil commented Nov 25, 2019

@rwjblue I don't mind either way, but I feel that this is a serious enough bug, that it might warrant an exception. Additionally:

  1. It is not a fix for Ember itself, but the build tooling, and other rules might apply.
  2. By not applying it, a previously supported platform (iOS 9) stops working completely.
  3. 3.8 has only just went into security maintenance mode with the 3.14 release 7 days ago.
  4. The bug has been known for a long time (see Version 3.10.1 regression: use of const in strict mode, causing critical errors in e.g. Safari 9 #18084).
  5. The bug was introduced during an active LTS 3.8 release cycle. The 3.8.1 LTS release works, but later releases fail due to a backport.
  6. It is a simple fix, and the effects can be verified by a diff of the dist files.

@rwjblue
Copy link
Member

rwjblue commented Nov 25, 2019

Restarted CI here

@timiyay
Copy link

timiyay commented Nov 26, 2019

I've been reviewing the Github comments related to iOS 9 crashes due to these leftover const declarations.

I was under the vague impression that the bug affects Ember 3.10 - 3.12, but @kanongil is saying it affects 3.8.2+ ? #18576 (comment)

Do we have a clear idea on the affected versions?

We're currently applying a hacky fix, and are hoping to see a backport of any fixes. I would completely support @kanongil's reasoning for backporting - this renders our Ember apps unloadable on previously-supported browsers.

We're targeting LTS versions in order to get these kind-of issues fixed and backported, and we'd need to rethink the usefulness of Ember's LTS if something like this is deemed unworthy of a backport, especially with the amount of Octane-related churn and confusion in the community at the moment.

@pzuraq
Copy link
Contributor

pzuraq commented Nov 26, 2019

@timiyay per @rwjblue's comment, our LTS support policy is that the current LTS version receives bugfixes such as these, and previous LTS versions only receive critical security fixes (e.g. issues that could endanger users). The recommendation is to upgrade to each LTS version, which is every 24 weeks, or slightly under half a year.

We work really hard to try to make sure that each LTS is as stable as possible, but unfortunately bugs do slip through, and we can't support them forever.

@pzuraq
Copy link
Contributor

pzuraq commented Nov 26, 2019

3.8 has only just went into security maintenance mode with the 3.14

My understanding was that it went into security mode with the 3.13 release, which is when 3.12 became an LTS, not 3.14.

@timiyay
Copy link

timiyay commented Nov 26, 2019

We're potentially trending towards "processes not people" here.

This is an major (major = app cannot load) bug, it has caused a lot of lost hours for developers. It appears to be a one-line fix.

There may be further context I'm not aware of. I've spent the past few days trying to learn the Ember build system enough to contribute a fix, but it's a steep learning curve.

If the answer is "sorry sir that's our policy" I can fully understand. It would also mean, given the gravity of this bug, that Ember's LTS scheme loses a lot of relevance. We'll likely have to go back to cherry-picking upgrade targets based on community vibe, as critical app-crashing bugs on LTS editions aren't prioritised over shiny new Octane features.

@pzuraq our hacky fix uses a similar addon-based solution to the https://github.com/pzuraq/ember-class-constructor-fix addon you authored. Do you think this PR could be restructured to this type of community addon, to enact a backport?

I couldn't find a suitable hook or seam in the Ember build to do this, which I why we've fallen back to a regex-based find-and-replace of const usage, which we're fairly uncomfortable with, but we need it to support iOS 9 users.

I would aspire to create such an addon to apply this fix, if I could get some guidance on how to apply it via an addon.

@pzuraq
Copy link
Contributor

pzuraq commented Nov 26, 2019

@timiyay if the ember-class-constructor-fix addon works, this fix likely will not fix your issue, as that fix was for an entirely different issue.

The original bug introduced in 3.8.2 was, ironically, probably trying to fix the same series of bugs. There have been a lot of these bugs, and we have backported a lot of different fixes. It has been a difficult process, and we finally completely rebuild the build pipeline in 3.13 to prevent all of them.

I understand the pain you're experiencing, believe me. That's why I spent days working to get the completely rebuilt build pipeline done, and that's why I backported many individual fixes to these issues when 3.8 was still active LTS.

Edit: And FWIW, it's also why I do intend to try to get the final fixes backported to 3.12 before the LTS window closes on it.

@timiyay
Copy link

timiyay commented Nov 26, 2019

@pzuraq ember-class-constructor-fix doesn't fix our issue, I was using it as an example of an Ember addon that can patch a build problem, thereby working around the need for a backport.

If 3.8 LTS isn't going to receive a backport, does that imply that 3.12 LTS will (or already has) fixed this issue? Is this PR destined for a 3.12 patch release? I've found this to be unclear from the handful of Github issue threads surrounding this class of problem.

My current understanding is that @pzuraq 's work to rebuild the pipeline, and consume an app's browser targets for the Ember source build, is available from 3.13, meaning that 3.12 would still suffer from the const bug. Is that accurate?

@pzuraq
Copy link
Contributor

pzuraq commented Nov 26, 2019

My intention is to backport those changes. We had them ready for 3.12, and landed them immediately in 3.13, so it should be pretty easy. There was also at least one followup PR for bugs caused by the changes, possibly more, and I haven’t had time to go through and collect them all just yet.

Would definitely appreciate someone taking the time to do it. I will probably be busy for the next few weeks, deep in the middle of the Glimmer VM upgrade, but if we haven’t gotten to it by then I’ll make sure to get it done before 3.17 is released. It’s important for ecosystem compatibility.

@timiyay
Copy link

timiyay commented Nov 26, 2019

@pzuraq I've reached out to you on Ember Discord to get a feel for the scope of the work to land your pipeline work in 3.12 LTS, to see if I can help out.

@kanongil
Copy link
Contributor Author

I personally have no need for a 3.8 backport, but I do plan to stay on 3.12 for a while, as 3.13+ is causing unacceptable performance regressions in my existing app. As such, I'd hope that it receives the backports that it needs.

Any chance this can be fixed in 3.12 soon? With my patch, or otherwise?

@rwjblue rwjblue merged commit eea4136 into emberjs:lts-3-12 Nov 26, 2019
@timiyay
Copy link

timiyay commented Nov 26, 2019

@kanongil I'm going to make an attempt to backport @pzuraq's build pipeline changes to 3.12 this week, after getting some tips from him yesterday via Discord.

I'll add a comment in this thread to link to any PRs I created.

@timiyay
Copy link

timiyay commented Nov 28, 2019

The proposed backport of the 3.13 dynamic build features is over at #18584

@rwjblue
Copy link
Member

rwjblue commented Dec 4, 2019

Kicked off a release of this in 3.12.2, should be published shortly

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

Successfully merging this pull request may close these issues.

4 participants