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

UI: Move the text encoding polyfill to a a proper detecting polyfill #4767

Merged
merged 1 commit into from
Oct 8, 2018

Conversation

johncowen
Copy link
Contributor

I spotted a huge bump in the size of my built output from a few weeks ago:

screen shot 2018-10-08 at 11 34 25

I followed it back to #4613 which was to add a different TextEncoder polyfill, but this wasn't exactly polyfilled. Even though certain browsers didn't need the polyfill, the polyfill was still in the bundled vendor.js file.

This PR splits this out into a proper polyfill. The TextEncoder polyfill is now only requested/downloaded if the browser you are using a browser that doesn't have a native implementation (basically IE).

Overall this puts my vendor.js file back to the size it was previously.

@johncowen johncowen added the theme/ui Anything related to the UI label Oct 8, 2018
@johncowen johncowen requested a review from a team October 8, 2018 10:41
@johncowen johncowen added this to the 1.3.0 milestone Oct 8, 2018
@@ -65,6 +65,8 @@ module.exports = function(defaults) {
// modules that you would like to import into your application
// please specify an object with the list of modules as keys
// along with the exports of each module as its value.
app.import('node_modules/text-encoding/lib/encoding-indexes.js', {outputFile: 'assets/encoding-indexes.js'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I've run across outputFile in ember-cli docs - that's really handy. 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vaguely remember reading about them in a GH issue, that was an issue saying they were missing from the documentation 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra note I figured on this yesterday, might be helpful if you ever need to use this. If you do this multiple times to the same outputFile it concats the sources together into the same file:

app.import('source-1', {outputFile: 'all.js'})
app.import('source-2', {outputFile: 'all.js'})

At some point 🔜 I'll probably merge the 2 files above into the same file to save a request.

Copy link
Collaborator

@meirish meirish left a comment

Choose a reason for hiding this comment

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

Nice! Awesome that you caught this.

@johncowen
Copy link
Contributor Author

👍 when I saw the bump my face was literally like that emoji 😮

@johncowen johncowen merged commit 8ba1c54 into master Oct 8, 2018
@johncowen johncowen deleted the feature/ui-proper-text-encoding-polyfill branch October 8, 2018 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants