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

Customizing build to exclude polyfill code #35

Closed
wants to merge 1 commit into from

Conversation

bradchoate
Copy link
Collaborator

  • Adds a new "maven_modules" directory with shims for the core-js-pure
    references that are used to polyfill for Array.includes, Array.from,
    Array.find, Array.findIndex, and String.includes.
  • Small change to webpack.config.js to search the maven_includes tree
    to resolve references ahead of node_modules.
  • Some updates to mavenAnalyticsAdapter.js to allow for a clean test
    suite (some lint errors were present).

ADS-4784

* Adds a new "maven_modules" directory with shims for the core-js-pure
  references that are used to polyfill for Array.includes, Array.from,
  Array.find, Array.findIndex, and String.includes.
* Small change to webpack.config.js to search the maven_includes tree
  to resolve references ahead of node_modules.
* Some updates to mavenAnalyticsAdapter.js to allow for a clean test
  suite (some lint errors were present).

ADS-4784
@bradchoate
Copy link
Collaborator Author

This change reduces the Prebid bundle by 28K (uncompressed)!

@bradchoate
Copy link
Collaborator Author

This does not replace the polyfill for Number.isInteger. The entire thing is part of Prebid's util.js module and looks like this:

export function isInteger(value) {
  if (Number.isInteger) {
    return Number.isInteger(value);
  } else {
    return typeof value === 'number' && isFinite(value) && Math.floor(value) === value;
  }
}

So, not a lot of code, compared to the third-party core-js polyfills.

Copy link

@vipulnaik vipulnaik left a comment

Choose a reason for hiding this comment

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

Hi @bradchoate, question: is there any change to the build process for building this for/in Phoenix, as described at https://github.com/themaven-net/tempest-phoenix/blob/master/htdocs/js/prebid/README.md#building-a-new-version?

@bradchoate
Copy link
Collaborator Author

@vipulnaik No, there's no change to the build process. Of course, if we aren't doing a polyfill for these by Phoenix itself, then we will lose support for IE11.

@AritraSengupta
Copy link

Should we remove this from webpack as well? (line 36-38)

module.resource && (allowedModules.src.concat(['core-js'])).some(
          name => module.resource.includes(path.resolve('./node_modules/' + name))
        )

Since we do not need to minify core-js anymore?

@AritraSengupta
Copy link

The rest looks good. Maybe we can ask Michelle we want to continue supporting IE 11 in Phoenix and then make a call.

@vipulnaik
Copy link

vipulnaik commented Aug 26, 2021

@AritraSengupta, @bradchoate that's a good point. Ideally we still want to keep IE 11 support, but we already have a system for loading global polyfills for older browsers, and if needed, we should add more polyfills there: https://github.com/themaven-net/tempest-phoenix/tree/master/htdocs/bootscripts/polyfills That way the extra polyfill code is downloaded only by IE 11 users, and it does not bloat the Prebid bundle.

So the question is: are the polyfills we are eliminating from Prebid already included? It looks like many of them are not. For instance, https://github.com/themaven-net/tempest-phoenix/blob/master/htdocs/bootscripts/polyfills/string/main.js has polyfills for startsWith and endsWith but does not have a polyfill for includes -- which is the one we are getting from core-js. I confirmed that if I load The Street on IE 11, string inclusion is not available for me in the IE 11 console. Similarly, array find and findIndex operations seem unsupported with the polyfills we are currently bringing into Phoenix on IE 11.

This means that, if we want full IE 11 support, parallel work may need to be done to expand our existing polyfills (at https://github.com/themaven-net/tempest-phoenix/tree/master/htdocs/bootscripts/polyfills). @bradchoate do you feel that would be within the scope of this ticket, or should we make a followup ticket for that work? In any case, depending on the level of IE 11 support we want to keep, we may have to block the update of Phoenix with this code until that work is done.

@bradchoate
Copy link
Collaborator Author

@vipulnaik Michelle says our monetization is so low for IE11 that it would be fine if we were to cut support for it. We already had some web component code in Phoenix that breaks under IE11 and haven't bothered to fix it due to lack of priority. Further, there are increasing runtime errors for ads delivered to IE11, where the ad payload itself is not backward compatible with IE11.

@bradchoate
Copy link
Collaborator Author

Should we remove this from webpack as well? (line 36-38)

@AritraSengupta I don't think it's necessary; I was trying to limit changes made to webpack.config.js as much as possible to reduce risk of conflicts with future merges from the upstream repo.

@vipulnaik vipulnaik self-requested a review August 27, 2021 14:34
Copy link

@vipulnaik vipulnaik left a comment

Choose a reason for hiding this comment

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

LGTM; can you make sure to pull the updated file into Phoenix, confirm the size reduction, and only close the ticket once the Phoenix side is deployed? Just want to make sure any impact of this is monitored right now rather than affecting somebody later when they need to pull an updated Prebid into Phoenix.

Thanks!

@bradchoate
Copy link
Collaborator Author

Merged to stable, stable-next branches manually.

@bradchoate bradchoate closed this Aug 27, 2021
santii7395 pushed a commit that referenced this pull request Jan 17, 2024
* add Rise adapter

* fixes

* change param isOrg to org

* Rise adapter

* change email for rise

* fix circle failed

* bump

* bump

* bump

* remove space

* Upgrade Rise adapter to 5.0

* added isWrapper param

* addes is_wrapper parameter to documentation

* added is_wrapper to test

* removed isWrapper

* Rise Bid Adapter: support Coppa param (#24)

* MinuteMedia Bid Adapter: support Coppa param (#25)

* Revert "MinuteMedia Bid Adapter: support Coppa param (#25)" (#26)

This reverts commit 66c4e7b.

* bump

* update coppa fetch

* setting coppa param update

* update Coppa tests

* update test naming

* Rise Bid Adapter: support plcmt and sua (#27)

* update minuteMediaBidAdapter - support missing params (#29)

* RIseBidAdapter: support currency (#35)

---------

Co-authored-by: Noam Tzuberi <noam.tzuberi@ironsrc.com>
Co-authored-by: noamtzu <noamtzu@gmail.com>
Co-authored-by: Noam Tzuberi <noamtzu@users.noreply.github.com>
Co-authored-by: Laslo Chechur <laslo.chechur@ironsrc.com>
Co-authored-by: OronW <41260031+OronW@users.noreply.github.com>
Co-authored-by: lasloche <62240785+lasloche@users.noreply.github.com>
Co-authored-by: inna <innayare@gmail.com>
Co-authored-by: YakirLavi <yakir.lavi@risecodes.com>
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