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 beta] Adds early Android versions to legacy target list #17246

Merged

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Dec 4, 2018

Currently, we only check for IE compatibility, but legacy Android browsers can also cause an issue. In general we need to solve this by respecting the browser targets of the client, but that will require more thorough changes to our build to actually use modules and drop bundling altogether.

lib/index.js Outdated Show resolved Hide resolved
@pzuraq pzuraq force-pushed the bugfix/add-android-to-legacy-targets branch 3 times, most recently from 45e7565 to b4d289b Compare December 6, 2018 00:30
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thank you, this seems much less brittle!

"chalk": "^2.3.0",
"ember-cli-babel": "^7.1.3",
Copy link
Member

Choose a reason for hiding this comment

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

are we sure that this does not cause any unintended side-effects? 1. because ember-cli-babel might do something at app-buildtime and 2. because it's Babel 7, but we're still using Babel 6 to build the Ember.js assets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that ember-cli-babel will only apply to addon source code, so ember-sources addon code, which doesn't exist. There's definitely the discrepancy with Babel 6, but since that is a _pre_publish step and fundamentally disconnected from ember-cli-babel, I figured it would make more sense to use the latest Babel instead. We can change this though if it seems problematic.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I just looked at https://unpkg.com/ember-source@3.5.1/lib/index.js, and this does indeed seem safe to do.

@rwjblue
Copy link
Member

rwjblue commented Dec 6, 2018

The browserstack failures seem related (this CI job):

Running: ./node_modules/.bin/testem ci -f testem.dist.js --host 127.0.0.1 --port 7774
not ok 1 IE 11.0 - [undefined ms] - Global error: Expected ':' at http://127.0.0.1:7774/288551829212/dist/ember.debug.js, line 182
    ---
        Log: |
            { type: 'error',
              testContext: {},
              text:
               'Expected \':\' at http://127.0.0.1:7774/288551829212/dist/ember.debug.js, line 182\n' }
    ...

@pzuraq pzuraq force-pushed the bugfix/add-android-to-legacy-targets branch from b4d289b to 6512a5a Compare December 6, 2018 16:31
@rwjblue rwjblue merged commit c281a65 into emberjs:master Dec 6, 2018
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.

3 participants