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

Webpack 4 production build is missing dom4 polyfill #3333

Closed
TheHolyWaffle opened this issue Feb 4, 2019 · 4 comments
Closed

Webpack 4 production build is missing dom4 polyfill #3333

TheHolyWaffle opened this issue Feb 4, 2019 · 4 comments

Comments

@TheHolyWaffle
Copy link

Environment

  • @blueprintjs/core 3.12.0
  • Windows 10 / Chrome 71

Steps to reproduce

  1. Do the following import:
    import { Position, Toaster, Intent } from "@blueprintjs/core";
  2. Make a production build using webpack 4
  3. Load and execute the page on IE11

Actual behavior

IE11 console errors: Object doesn't support property or method 'remove'

Expected behavior

No error should appear

Possible solution

It appears that the dom4 polyfill that includes the .remove() functionality is not included during the production build. This is because @blueprintjs/core is marked as sideEffect: false in package.json. Which means that webpack will cut away any unused imports/exports. Including the following lines which are responsible for loading in dom4:

if (typeof window !== "undefined" && typeof document !== "undefined") {
// we're in browser
// tslint:disable-next-line:no-var-requires
require("dom4"); // only import actual dom4 if we're in the browser (not server-compatible)
// we'll still need dom4 types for the TypeScript to compile, these are included in package.json
}

Solution is to mark this file as a sideEffect, see https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

@giladgray
Copy link
Contributor

@TheHolyWaffle nice find. interesting in submitting a PR?

@TheHolyWaffle
Copy link
Author

@TheHolyWaffle I'll leave that up to the maintainers :)

@tom-s-powell
Copy link
Contributor

This is fixed in #3868 right?

@adidahiya
Copy link
Contributor

@tom-s-powell yes, I believe so, thanks. fixed by #3868 & #3876

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants