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

Problems with Handsontable 7 and core-js in ES build #5888

Closed
aaronbeall opened this issue Mar 13, 2019 · 6 comments
Closed

Problems with Handsontable 7 and core-js in ES build #5888

aaronbeall opened this issue Mar 13, 2019 · 6 comments
Assignees
Milestone

Comments

@aaronbeall
Copy link
Contributor

@aaronbeall aaronbeall commented Mar 13, 2019

Description

After upgrading to Handsontable 7 our webpack build fails with errors like this:

ERROR in ./node_modules/handsontable/es/3rdparty/walkontable/src/index.js
Module not found: Error: Can't resolve 'core-js/modules/web.dom.iterable' in '/Users/abeall/Documents/temp/ht7/node_modules/handsontable/es/3rdparty/walkontable/src'
@ ./node_modules/handsontable/es/3rdparty/walkontable/src/index.js 53:0-42
@ ./node_modules/handsontable/es/core.js
@ ./node_modules/handsontable/es/index.js
@ ./src/index.js

In our case we are using core-js@beta but you also get this problem with a fresh project that hasn't installed the core-js that Handsontable ES build depends on.

Steps to reproduce

  1. Create a fresh repo with npm init
  2. npm install webpack webpack-cli handsontable
  3. Add src/index.js with this content:
import Handsontable from "handsontable"
console.log(Handsontable)
  1. Run webpack (./node_modules/.bin/webpack)

Result is a bunch of errors like above: "can't resolve core-js/..." modules.

Workaround

If you npm install core-js it builds without error. However, in my project I already use a different version of core-js so I can't just do that. I need to down-grade core-js to the version that Handsontable apparently expects.

Problems

I think there are two related issues here:

  1. The ES build has imports for core-js but package.json does not declare core-js as a dependency. This results in a fresh install having a hidden dependency on a specific version of core-js when using the ES version. (The bundled dist version doesn't have this problem.)
  2. The source code imports @babel/polyfill which has side-effects. I don't think it should do this. I think the host project is responsible for polyfills. For example in our case, we already use a different version of core-js, even if point 1 was resolved it's not clear what the end result would be with @babel/polyfill vs our own import of core-js, since they aren't the same but both have side-effects on global APIs.

Fix?

  1. Declare core-js as a dependency in package.json.
  2. import "@babel/runtime" or import nothing and rely on host app to polyfill for their target browsers. Probably requires some documentation about polyfill requirements, something similar to React docs.

Thoughts? At the moment we are down-grading to an older version of core-js (which requires refactoring code to not use features that were in the newer version) so that we can build with Handsontable 7.

@robcecil

This comment has been minimized.

Copy link

@robcecil robcecil commented Mar 15, 2019

I am blocked using the 7.0.0 version and will instead use 6.2.2 (with React) for the time being.

@wojciechczerniak wojciechczerniak added this to the March 2019 milestone Mar 19, 2019
@AMBudnik

This comment has been minimized.

Copy link
Contributor

@AMBudnik AMBudnik commented Mar 21, 2019

Thank you for reporting the issue.

It is scheduled for March. I hope that we'll be able to fix it soon.

@AMBudnik AMBudnik added the Type: Bug label Mar 21, 2019
@wojciechczerniak

This comment has been minimized.

Copy link
Member

@wojciechczerniak wojciechczerniak commented Mar 21, 2019

TLDR;

To restore webpack support prior v7.0.0 import Handsontable UMD module:

import Handsontable from `handsontable/dist/handsontable.js`;

With wrapper an alias in webpack.conf.js is the only option:

"resolve": {
    "alias": {
        "handsontable": "node_modules/handsontable/dist/handsontable.js"
    }
}

What happened

We've removed browser field from package.json. Which by default is read by Webpack before module and main fields. Previous (browser field) configuration created a problem where final app had overhead of polyfills and Babel helpers loaded by Handsontable. And I agree with @aaronbeall on this, it's should be the application developer who decide how to handle polyfills.

We didn't liked the idea of import "@babel/polyfill" either.

What can be done

Already mentioned npm install core-js fixes the issue for most. Or they already have it installed, or their framework does so they didn't even noticed. We didn't foreseen that someone uses different version of core-js already. Sorry for that @aaronbeall @robcecil.

The fix for both is adding core-js as a dependency. We're checking this option.

budnix added a commit that referenced this issue Mar 22, 2019
Add core-js (polyfill for ES6+ features) to the production dependencies
and changed how these polyfills are included using a "usage" strategy
where babel packages decide what module should be injected to support
our minimal browser support.

Notice: After these changes, the dist files are bigger by 55kB for
`.full.js` files and about 12kB bigger for minified (`.min.js`)
versions.

Issue: #5888
@budnix budnix mentioned this issue Mar 22, 2019
3 of 6 tasks complete
budnix added a commit that referenced this issue Mar 28, 2019
Issue: #5888
budnix added a commit that referenced this issue Mar 28, 2019
Add core-js (polyfill for ES6+ features) to the production dependencies
and changed how these polyfills are included using a "usage" strategy
where babel packages decide what module should be injected to support
our minimal browser support.

Notice: After these changes, the dist files are smaller by 100kB 
(after upgrade webpack to v4) for `.full.js` files and about 14kB bigger 
for minified (`.min.js`) versions.

Issue: #5888
@aaronbeall

This comment has been minimized.

Copy link
Contributor Author

@aaronbeall aaronbeall commented Apr 2, 2019

@wojciechczerniak

This comment has been minimized.

Copy link
Member

@wojciechczerniak wojciechczerniak commented Apr 3, 2019

It was already released few days later when we were fixing the issue 😉

We're using core-js@3 https://github.com/handsontable/handsontable/pull/5908/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R65

And we have release branch for 7.0.1 if you want to test it: https://github.com/handsontable/handsontable/tree/release/7.0.1

@AMBudnik

This comment has been minimized.

Copy link
Contributor

@AMBudnik AMBudnik commented Apr 8, 2019

Officially added to Handsontable 7.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.