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] Compile Ember dynamically in consuming applications #18208

Merged
merged 2 commits into from
Aug 15, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Jul 17, 2019

  • Phase 1, prepublish: Run Typescript and Rollup on the main Ember
    packages, and strip out canary-features. Publish these packages, along
    with the dependencies for Ember, in the dist/ folder.

  • Phase 2, in addon: Run Babel transpilation using the consumer's
    ember-cli-babel and Babel configuration on the dist packages and
    dependencies, and bundle up the final ember.js file to serve to
    apps. This also includes debug flags and, in theory, svelting.

Two major changes that will occur because of this:

  1. We will no longer be distributing ember.prod.js, ember.debug.js,
    ember.min.js, or ember-testing.js. These files existing may be
    something that people rely on, and the packages that are
    distributed aren't quite ready to build in a "normal" way, using
    webpack or another bundler.
  2. We will no longer be building ember.prod.js, ember.min.js, or
    ember.debug.js at all, only a single ember.js file. We no longer
    need separate builds, because the environment settings of the build
    will handle the differences for us.

Unfortunately, until we disentangle ember-cli from the implementation
details of ember-source we cannot convert Ember to build like a
normal addon locally. Using the EmberAddon class blows up in many
small ways.

@rwjblue
Copy link
Member

rwjblue commented Jul 22, 2019

Looks like the CI failures are related:

not ok 1 App Boot > App boots and routes to a URL
  ---
  message: "Died on test #1     at Object.<anonymous> (/home/travis/build/emberjs/ember.js/tests/node/app-boot-test.js:8:9): Could not find module `url` imported from `(require)`"
  severity: failed
  actual: null
  expected: undefined
  stack: Error: Could not find module `url` imported from `(require)`
    at missingModule (/home/travis/build/emberjs/ember.js/dist/ember.js:247:11)
    at findModule (/home/travis/build/emberjs/ember.js/dist/ember.js:258:7)
    at requireModule (/home/travis/build/emberjs/ember.js/dist/ember.js:24:15)
    at installProtocolForURL (/home/travis/build/emberjs/ember.js/dist/ember.js:6561:17)
    at new Environment$1 (/home/travis/build/emberjs/ember.js/dist/ember.js:6601:7)
    at Function.create (/home/travis/build/emberjs/ember.js/dist/ember.js:6613:14)
    at FactoryManager.create (/home/travis/build/emberjs/ember.js/dist/ember.js:1125:33)
    at instantiateFactory (/home/travis/build/emberjs/ember.js/dist/ember.js:929:63)
    at _lookup (/home/travis/build/emberjs/ember.js/dist/ember.js:861:12)
    at processInjections (/home/travis/build/emberjs/ember.js/dist/ember.js:969:26)

@runspired
Copy link
Contributor

Just a heads up, this PR may not be enough.

Currently in ember-source 3.11 when native extends is mixed with .extend travis-web fails against ember-data when:

  • targets includes "last 2 edge"
  • AND targets does NOT include IE11

It passes when:

  • targets does not include edge

  • AND targets does NOT include IE11

  • targets includes "last 1 edge"

  • AND targets does NOT include IE11

  • targets does not include "edge" or includes "last 1 edge"/"last 2 edge"

  • AND targets INCLUDES IE11

You can reproduce this by:

  • cloning ember-data
  • running yarn install
  • running node ./bin/packages-for-commit.js
  • running yarn test-external:travis-web --skip-smoke-test

This test will fail and you will find travis-web in the directory ../__external-test-cache/travis-web in a state wherein you may alter the targets.js file and CI status as desired and run tests as normal in an ember app.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jul 27, 2019

Based on your description @runspired, this PR should fix those issues as well. I believe the problem is the same as #18084, where specific targets are causing us to ship the wrong build and causing the conflict. With this PR, we will be compiling Ember using the same targets as the consuming application, so this type of conflict won't be possible anymore.

@BnitoBzh
Copy link

Hello everyone, when do you think this problem can be solved? We need our production applications to run on ios9, now for several weeks we are having trouble. The only solution we have now is to edit the build of applications after each deployment by replacing all const with var.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jul 30, 2019

Hey, sorry, have been working on perf tuning for tracked properties in 3.13 this last week. I'm wrapping that up, should be able to get to this sometime this week.

@pzuraq pzuraq force-pushed the bugfix/compile-ember-dynamically branch 6 times, most recently from 810550e to de69708 Compare August 2, 2019 17:26
@pzuraq pzuraq force-pushed the bugfix/compile-ember-dynamically branch from de69708 to 2ac757a Compare August 9, 2019 21:03
@pzuraq pzuraq force-pushed the bugfix/compile-ember-dynamically branch 7 times, most recently from b4de6b7 to e1c0e0c Compare August 13, 2019 15:55
Chris Garrett added 2 commits August 14, 2019 11:33
This PR sets up Ember to do a two phase build:

* **Phase 1, prepublish:** Run Typescript and Rollup on the main Ember
  packages, and strip out canary-features. Publish these packages, along
  with the dependencies for Ember, in the `dist/` folder.

* **Phase 2, in addon:** Run Babel transpilation using the consumer's
  `ember-cli-babel` and Babel configuration on the dist packages and
  dependencies, and bundle up the final `ember.js` file to serve to
  apps. This also includes `debug` flags and, in theory, svelting.

Two major changes that will occur because of this:

1. We will no longer be distributing `ember.prod.js`, `ember.debug.js`,
   `ember.min.js`, or `ember-testing.js`. These files existing may be
   something that people rely on, and the packages that _are_
   distributed aren't quite ready to build in a "normal" way, using
   webpack or another bundler.
2. We will no longer be _building_ `ember.prod.js`, `ember.min.js`, or
   `ember.debug.js` at all, only a single `ember.js` file. We no longer
   need separate builds, because the environment settings of the build
   will handle the differences for us.

We _are_ continuing to distribute a pre-built version of the `ember.js`
and `ember-testing.js` files. These are used only in the case where the
build targets match the default development build targets for Ember
apps, providing a small optimization for users' dev workflow.

Unfortunately, until we disentangle `ember-cli` from the implementation
details of `ember-source` we cannot convert Ember to _build_ like a
normal addon locally. Using the `EmberAddon` class blows up in many
small ways.
@pzuraq pzuraq force-pushed the bugfix/compile-ember-dynamically branch from e1c0e0c to e526a04 Compare August 14, 2019 18:34

/**
* There isn't a way for us to override targets through ember-cli-babel, and we
* don't want to introduce that functionality for reasons. This is a quick and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to address this over in ember-cli-babel (in a follow up), would you mind making an issue over there about the general use case?

@rwjblue rwjblue merged commit af48faa into master Aug 15, 2019
@rwjblue rwjblue deleted the bugfix/compile-ember-dynamically branch August 15, 2019 20:19
@BnitoBzh
Copy link

Hi ! is this pull request can be merged into ember 3.12.x or 3.11.x ?

@pzuraq
Copy link
Contributor Author

pzuraq commented Aug 15, 2019

This is marked for backport to LTS, so it will be backported to 3.12 for sure

if (!isProduction && PRE_BUILT_TARGETS.every(target => targets.includes(target))) {
ember = new Funnel(tree, {
destDir: 'ember',
include: ['ember.debug.js', 'ember.debug.map', 'ember-testing.js', 'ember-testing.map'],
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ember-template-compiler.js be present here as well?

This change might have caused this: embroider-build/embroider#311.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems plausible, mind making a PR so we can focus discussion over there?

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's done at #18291.

@Turbo87
Copy link
Member

Turbo87 commented Aug 26, 2019

why is this PR marked as "BUGFIX lts"? the description is not mentioning any bugs that are being fixed by this and it seems like a pretty significant change for those that expect LTS to work the same way as before 🤔

@rwjblue
Copy link
Member

rwjblue commented Aug 26, 2019

@Turbo87 - There is a whole category of issues that this PR will ultimately resolve. A few examples:

These changes are currently only in beta (3.13.0-beta.3 I think?), and will only be backported to LTS (likely 3.12 if we do it, not 3.8) if we can be extremely certain that they will not cause more issues than it solves.

@snewcomer
Copy link
Contributor

Has this been backported to 3.12? Is this something I can help with. This is the internal addon we have to fix this temporarily (we are on 3.12)

'use strict';
const replace = require('broccoli-replace');

module.exports = {
    name: 'temporary-ember-fix',
    postprocessTree(type, tree) {
        if (type !== 'all') {
            return tree;
        }

        return replace(tree, {
            files: ['**/vendor*.js'],
            patterns: [{
                match: /const\s/g,
                replacement: 'var '
            }]
        });
    },

    isDevelopingAddon() {
        return true;
    }
};

@pzuraq
Copy link
Contributor Author

pzuraq commented Oct 18, 2019

I don't believe it has yet, but it definitely should be. It's a large change, but it's been pretty stable so far, no more issues, and it shouldn't be too hard to pull it onto 3.12 (originally we were hoping to get it in then, but we decided against it since we didn't have enough time for testing).

If you can take a crack at it, that'd be very helpful, otherwise I'll try to find some time in the near future to get it going.

Copy link
Member

rwjblue commented Oct 18, 2019

Ya, the major thing to do is identify all the bugfix commits for these files that happened after this initially landed so we have a clearer idea of what to backport.

@ssnielsen
Copy link

Very glad to see that this has/is being worked on :) We've recently upgraded to 3.12.0 and have had reports from our iOS 9 users about the issue mentioned in #18084

@rwjblue What do you estimate is the timeframe wrt. getting this fix into a future 3.12.x release?

Copy link
Member

rwjblue commented Nov 1, 2019

TBH, I don't know. We really need to figure out the full list of changes needed to backport, that is probably the main blocker. If someone has time to dig through the commit history for anything build related since this PR landed and make a PR targetting the lts-3-12 branch that would be super helpful and make it massively more likely for it to happen "soon". After that actually doing the release is pretty simple once we get the changes there.

@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 1, 2019

FWIW I made https://github.com/pzuraq/ember-class-constructor-fix which should fix most of the general issues in older builds. It's a more general fix too, which means it'll work prior to 3.12 as well.

#18084 is particularly strange though, since it appears that the legacy build is not being used, despite very clear legacy targets. Will comment over there to try to get more details.

@snewcomer
Copy link
Contributor

snewcomer commented Nov 1, 2019

This is phenomenal! I love the approach. Is this the fix or is backporting changes the fix? Seems like the former? (if the latter, I can help today and this weekend)

@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 4, 2019

I think backporting would still be useful, if possible, but I don't think we need to spend extra cycles on it. This fix should work in any cases where we eagerly opt into the legacy build even if we shouldn't actually do that, and as I was working through the fix I thought that was the only possible thing that could accidentally happen.

This is why #18084 is so strange to me, based on the code I don't think it should be possible. We basically only opt-into legacy IFF classes need to be compiled OR another feature needs to be compiled, so

  1. If classes are compiled, then the legacy build is used, and user's classes are compiled, so nothing to fix.
  2. If classes are not compiled, and nothing else triggers the legacy build, then there's nothing to fix.
  3. If classes are not compiled, but something else is and triggers the legacy build, then user code is not compiled, but framework code is, and the fix patches it.

I don't think it should be possible for user classes to be compiled, and for the framework to not be the legacy build, but maybe there's something I wasn't considering.

timiyay pushed a commit to timiyay/ember.js that referenced this pull request Nov 27, 2019
This PR sets up Ember to do a two phase build:

* **Phase 1, prepublish:** Run Typescript and Rollup on the main Ember
  packages, and strip out canary-features. Publish these packages, along
  with the dependencies for Ember, in the `dist/` folder.

* **Phase 2, in addon:** Run Babel transpilation using the consumer's
  `ember-cli-babel` and Babel configuration on the dist packages and
  dependencies, and bundle up the final `ember.js` file to serve to
  apps. This also includes `debug` flags and, in theory, svelting.

Two major changes that will occur because of this:

1. We will no longer be distributing `ember.prod.js`, `ember.debug.js`,
   `ember.min.js`, or `ember-testing.js`. These files existing may be
   something that people rely on, and the packages that _are_
   distributed aren't quite ready to build in a "normal" way, using
   webpack or another bundler.
2. We will no longer be _building_ `ember.prod.js`, `ember.min.js`, or
   `ember.debug.js` at all, only a single `ember.js` file. We no longer
   need separate builds, because the environment settings of the build
   will handle the differences for us.

We _are_ continuing to distribute a pre-built version of the `ember.js`
and `ember-testing.js` files. These are used only in the case where the
build targets match the default development build targets for Ember
apps, providing a small optimization for users' dev workflow.

Unfortunately, until we disentangle `ember-cli` from the implementation
details of `ember-source` we cannot convert Ember to _build_ like a
normal addon locally. Using the `EmberAddon` class blows up in many
small ways.

[BUGFIX lts] Updates based on feedback

Cherry-picked from PR emberjs#18208
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.

8 participants