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

perf: remove default array/object polyfills #3641

Merged

Conversation

jacobmllr95
Copy link
Member

@jacobmllr95 jacobmllr95 commented Jul 8, 2019

Describe the PR

This PR removes the default polyfills for:

  • Array.from()
  • Array.isArray()
  • Object.assign()
  • Object.is()

This should not be a breaking change since our polyfill recommendations in the docs should catch those anyway (for IE 11 and older android)

PR checklist

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Enhancement
  • ARIA accessibility
  • Documentation update
  • Other (please describe)

Does this PR introduce a breaking change? (check one)

  • No
  • Yes (please describe)

The PR fulfills these requirements:

  • It's submitted to the dev branch, not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (i.e. [...] (fixes #xxx[,#xxx]), where "xxx" is the issue number)
  • It should address only one issue or feature. If adding multiple features or fixing a bug and adding a new feature, break them into separate PRs if at all possible.
  • The title should follow the Conventional Commits naming convention (i.e. fix(alert): not alerting during SSR render, docs(badge): update pill examples, fix typos, chore: fix typo in README, etc). This is very important, as the CHANGELOG is generated from these messages.

If new features/enhancement/fixes are added or changed:

  • Includes documentation updates (including updating the component's package.json for slot and event changes)
  • New/updated tests are included and passing (if required)
  • Existing test suites are passing
  • The changes have not impacted the functionality of other components or directives
  • ARIA Accessibility has been taken into consideration (Does it affect screen reader users or keyboard only users? Clickable items should be in the tab index, etc.)

If adding a new feature, or changing the functionality of an existing feature, the PR's
description above includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@codecov
Copy link

codecov bot commented Jul 8, 2019

Codecov Report

Merging #3641 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #3641   +/-   ##
=======================================
  Coverage   99.26%   99.26%           
=======================================
  Files         224      224           
  Lines        4326     4326           
  Branches     1231     1227    -4     
=======================================
  Hits         4294     4294           
  Misses         26       26           
  Partials        6        6
Impacted Files Coverage Δ
src/utils/array.js 100% <100%> (ø) ⬆️
src/utils/object.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3641ba...1403019. Read the comment docs.

@tmorehouse tmorehouse added this to In Progress in 2.0.0 Stable via automation Jul 8, 2019
@jacobmllr95 jacobmllr95 marked this pull request as ready for review July 8, 2019 21:30
@jacobmllr95
Copy link
Member Author

@tmorehouse Can you please test the deployment preview on IE 11 and confirm that everything is still working as it should?

@tmorehouse tmorehouse merged commit 8b34bf2 into bootstrap-vue:dev Jul 8, 2019
2.0.0 Stable automation moved this from In Progress to Completed Jul 8, 2019
@phil-w
Copy link
Contributor

phil-w commented Jul 19, 2019

Yeah, that removal of the polyfill for Array.from() killed my user's IE11. So it goes.

I'm using latest everything (more fool me), but the polyfill is:
loader: 'babel-loader', options: { presets: ['@babel/preset-env'] }

That's not precisely what you suggest, so I guess I'll try to change it around. As far as I can tell I do have _array.from() in my code, so it's being polyfilled from somewhere.... but it's definitely not converting a dom object into an array, which I think is what you're trying to do in b-tabs / selectAll / arrayFrom

Does anyone have an @babel/preset-env configuration which works with this new code?


No worries, I switched to your recommendation to use Polyfill.io (so I hit IE11 users with extra stuff only!) and that works ok again.

@edwh
Copy link

edwh commented Jan 18, 2020

I've had a problem which I think is due to this. esm/utils/array.js take a local copy of Array.from, which it exports for use in other code. If Array.from has not yet been polyfilled, then even if it's polyfilled later the code will still use the undefined version.

I'm seeing this in a Nuxt environment where I'm having difficulty getting the polyfill to execute before the array.js code does.

I could be wrong, but isn't it a bad idea to take a copy of a function you expect to be polyfilled? Would it be better to use Array.from etc directly? It feels like old code which exists because we used to have polyfill implementations in there.

FreegleGeeks pushed a commit to Freegle/iznik-nuxt that referenced this pull request Jan 18, 2020
…revents polyfills. This is a workaround by patching

the results of the nuxt build until I have something better.  More details here:
bootstrap-vue/bootstrap-vue#3641
https://cmty.app/nuxt/nuxt.js/issues/c10213
@tmorehouse
Copy link
Member

We use references to the often used methods like "Array.from", as javascript minifiers will minify a variable name down to one or two characters, where it normally would leave "Array.from" if used directly.

Polyfills should normally be loaded before any other code is loaded into a browser.

@edwh
Copy link

edwh commented Jan 18, 2020

Thanks for your reply - interesting about the minifier.

Do you know how I can achieve getting the polyfill in there first in Nuxt? I've tried:

  • a plugin
  • a module
  • a script tag

...but bootstrap vue gets in there first. I presume the nuxt build adds the plugins from packages first.

I've opened an issue over there in case I'm missing something:

https://cmty.app/nuxt/nuxt.js/issues/c10213

@tmorehouse
Copy link
Member

For polyfills, you could load them in nuxt.config.js:

module.exports = {
  // ...
  head: {
    meta: [{ 'http-equiv': 'X-UA-Compatible', content: 'IE=edge' }],
    script: [
      {
        src: '//polyfill.io/v3/polyfill.min.js?features=es2015%2CIntersectionObserver',
        crossorigin: 'anonymous'
      }
    ]
  },
  // ...
}

polyfill.io only sends the required polyfills to the browser based on its user agent string.

@tmorehouse
Copy link
Member

tmorehouse commented Jan 18, 2020

Could you show a copy of your nuxt.config.js? Specifically your plugins and modules entry?

It might be that Nuxt processes modules before plugins.

How are you creating your polyfill plugin? as a module or a plugin?

tmorehouse added a commit that referenced this pull request Jan 18, 2020
My making each shortcut a method, it allows for handling of late loaded polyfills.

See #3641 (comment)
@edwh
Copy link

edwh commented Jan 18, 2020

I tried the script method, but I can see from a console log in array.js that Array.from is undefined at that point. I can also see from the Network tab that the npm scripts are loaded before the polyfill one is.

My nuxt.config is a bit long, but here's the modules section:

  modules: [
    'nuxt-polyfill',
    '@nuxtjs/sitemap',
    '@nuxtjs/sentry',
    'bootstrap-vue/nuxt',
    'nuxt-rfg-icon',
    '@nuxtjs/axios',
    '@nuxtjs/pwa',
    'nuxt-dayjs-module',
    '@nuxtjs/redirect-module',
    [
      '@nuxtjs/component-cache',
      {
        max: 1000,
        maxAge: 300 * 60 * 60
      }
    ]
],

Lots of plugins, but here's the start:

  plugins: [
    '~/plugins/polyfills',

    // Our template formatting utils.
    '~/plugins/filters',

    // Our directives
    '~/plugins/directives',

The resulting index.js built shows bootstrap-vue loaded very early on, before I can get any of my stuff in there:

import nuxt_plugin_workbox_6720dc58 from 'nuxt_plugin_workbox_6720dc58' // Source: .\\workbox.js (mode: 'client')
import nuxt_plugin_nuxticons_014eaee8 from 'nuxt_plugin_nuxticons_014eaee8' // Source: .\\nuxt-icons.js (mode: 'all')
import nuxt_plugin_bootstrapvue_4e942f2a from 'nuxt_plugin_bootstrapvue_4e942f2a' // Source: .\\bootstrap-vue.js (mode: 'all')
import nuxt_plugin_dayjsplugin_2bb79721 from 'nuxt_plugin_dayjsplugin_2bb79721' // Source: .\\dayjs-plugin.js (mode: 'all')
import nuxt_plugin_axios_410df388 from 'nuxt_plugin_axios_410df388' // Source: .\\axios.js (mode: 'all')
import nuxt_plugin_sentryserver_b8e26c00 from 'nuxt_plugin_sentryserver_b8e26c00' // Source: .\\sentry.server.js (mode: 'server')
import nuxt_plugin_sentryclient_3dd55878 from 'nuxt_plugin_sentryclient_3dd55878' // Source: .\\sentry.client.js (mode: 'client')
import nuxt_plugin_intersectionobserver_511bbcfb from 'nuxt_plugin_intersectionobserver_511bbcfb' // Source: .\\nuxt-polyfill\\intersection-observer.js (mode: 'all')
import nuxt_plugin_arrayfrompolyfill_493cc412 from 'nuxt_plugin_arrayfrompolyfill_493cc412' // Source: .\\nuxt-polyfill\\array-from-polyfill.js (mode: 'all')
import nuxt_plugin_customeventpolyfill_4c0d3117 from 'nuxt_plugin_customeventpolyfill_4c0d3117' // Source: .\\nuxt-polyfill\\custom-event-polyfill.js (mode: 'all')
import nuxt_plugin_eventpolyfill_7b199f12 from 'nuxt_plugin_eventpolyfill_7b199f12' // Source: .\\nuxt-polyfill\\event-polyfill.js (mode: 'all')
import nuxt_plugin_promisepolyfill_27958cf8 from 'nuxt_plugin_promisepolyfill_27958cf8' // Source: .\\nuxt-polyfill\\promise-polyfill.js (mode: 'all')
import nuxt_plugin_mutationobserver_cc75d7ca from 'nuxt_plugin_mutationobserver_cc75d7ca' // Source: .\\nuxt-polyfill\\mutation-observer.js (mode: 'all')
import nuxt_plugin_googleanalytics_3631546a from 'nuxt_plugin_googleanalytics_3631546a' // Source: .\\google-analytics.js (mode: 'client')
import nuxt_plugin_polyfills_7dce8c4e from 'nuxt_plugin_polyfills_7dce8c4e' // Source: ..\\plugins\\polyfills (mode: 'all')

So I can't find any way to get a polyfill in there early enough (there may be one, I just don't know what it is).

@tmorehouse
Copy link
Member

What is the difference between 'nuxt-polyfill' and '~/plugins/polyfills'? is the latter providing something that the former isnt'?

@edwh
Copy link

edwh commented Jan 18, 2020 via email

jacobmllr95 added a commit that referenced this pull request Jan 21, 2020
…ndling late loaded polyfills (#4647)

* chore(utils): pass all Array/Object shortcuts as functions


My making each shortcut a method, it allows for handling of late loaded polyfills.

See #3641 (comment)

* Update object.js

* Update object.js

* Update dom.js

* Update dom.js

* Update dom.js

Co-authored-by: Jacob Müller <jacob.mueller.elz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
2.0.0 Stable
  
Completed
Development

Successfully merging this pull request may close these issues.

None yet

4 participants