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

ES5 bundle conflict with core-js v3 #419

Closed
robertknight opened this issue Mar 26, 2019 · 23 comments · Fixed by #432
Closed

ES5 bundle conflict with core-js v3 #419

robertknight opened this issue Mar 26, 2019 · 23 comments · Fixed by #432

Comments

@robertknight
Copy link

@robertknight robertknight commented Mar 26, 2019

The core-js project released a new version, v3.0.0, which changes the module layout. The generated fetch-mock es5 bundle in es5/lib.js requires core-js modules assuming core-js v2's layout. As a result, attempting to include fetch-mock in a browserify-generated bundle (eg. for use with a browser-based test runner such as Karma) fails.

Steps to reproduce

  1. Save the following package.json into an empty directory:
{
  "name": "fetch-mock-corejs-v3-test",
  "version": "1.0.0",
  "devDependencies": {
    "browserify": "^16.2.3",
    "core-js": "^3.0.0",
    "fetch-mock": "^7.3.1"
  }
}
  1. Run npm install
  2. Run ./node_modules/.bin/browserify -r fetch-mock

Output

Error: Cannot find module 'core-js/modules/es6.typed.array-buffer' from '/private/tmp/fetch-mock-corejs-v3-test/node_modules/fetch-mock/es5'
    at /private/tmp/fetch-mock-corejs-v3-test/node_modules/browser-resolve/node_modules/resolve/lib/async.js:46:17
    at process (/private/tmp/fetch-mock-corejs-v3-test/node_modules/browser-resolve/node_modules/resolve/lib/async.js:173:43)
    at ondir (/private/tmp/fetch-mock-corejs-v3-test/node_modules/browser-resolve/node_modules/resolve/lib/async.js:188:17)
    at load (/private/tmp/fetch-mock-corejs-v3-test/node_modules/browser-resolve/node_modules/resolve/lib/async.js:69:43)
    at onex (/private/tmp/fetch-mock-corejs-v3-test/node_modules/browser-resolve/node_modules/resolve/lib/async.js:92:31)
    at /private/tmp/fetch-mock-corejs-v3-test/node_modules/browser-resolve/node_modules/resolve/lib/async.js:22:47
    at FSReqCallback.oncomplete (fs.js:158:21)

Notes

My suggested solution here would be to omit polyfills from fetch-mock itself and instead make it the responsibility of the package consumer to include polyfills, at least for ES standard library functionality. This avoids potential conflicts with other polyfills and in some cases will reduce bundle size.

Thoughts? I would be happy to submit a PR if the proposed change is agreeable.

@wheresrhys

This comment has been minimized.

Copy link
Owner

@wheresrhys wheresrhys commented Mar 27, 2019

In the past I didn't prebundle polyfill, but there were so many issues raised relating to it which eventually meant I caved in and included it. In principle, I agree that it's the consumer's responsibility to polyfill, but the reality is that there are scenarios where that's not so easy.

The best solution - unless you can think of a better one - is to generate a new core-js 3 compatible bundle, save it as a new file name, and document that users in a core-js 3 environment will need to reference that exact file. In the next major version that could switch to being the default browser export, with a core-js 2 bundle being available for backwards compatibility

It's kinda tricky to implement as will require multiple builds, with different npm dependencies, within the publish step. If you're familiar with circleci a PR to implement it would be welcome, otherwise I will try to get round to it sometime.

@robertknight

This comment has been minimized.

Copy link
Author

@robertknight robertknight commented Mar 28, 2019

In principle, I agree that it's the consumer's responsibility to polyfill, but the reality is that there are scenarios where that's not so easy.

Do you have some examples of issues that have come up in the past?

Generating a bundle with core-js v3 would resolve my immediate problem but it would still result in a larger bundle than necessary, which slows down the test runner.

@gfortaine

This comment has been minimized.

Copy link

@gfortaine gfortaine commented Apr 8, 2019

Hello,

+1 for this issue.

Our immediate workaround (using Karma) has been to load node_modules/fetch-mock/dist/es5/client-bundle.js from Files :

files: [
  "node_modules/fetch-mock/dist/es5/client-bundle.js" // http://www.wheresrhys.co.uk/fetch-mock/#usageinstallation
],

Très cordialement / Best Regards,

@thommyhh

This comment has been minimized.

Copy link

@thommyhh thommyhh commented Apr 13, 2019

I would vote for a v8.0.0 which depends on core-js v3.x and so requires modules from the v3.x layout.

@thommyhh

This comment has been minimized.

Copy link

@thommyhh thommyhh commented Apr 13, 2019

Another workaround that might help some people. I noticed, that the structure is different when I load the package from git instead of npm/yarn. Instead of require('fetch-mock') I need to use require('fetch-mock/src/client'). The big difference is, that there are no dependencies on any core-js modules when I include the package via git. So how do they get in there in the packaged version, when they obviously aren't needed?

@grug

This comment has been minimized.

Copy link
Collaborator

@grug grug commented May 29, 2019

I suspect this will get more interesting, as Angular 8.0 came out today which means that people can't use fetch-mock in Angular 8.0+ as it stands.

@wheresrhys I'd love to help out on this issue. Would you like to work on this together? IIRC you're in London - happy to grab a beer with you to talk it over.

@wheresrhys

This comment has been minimized.

Copy link
Owner

@wheresrhys wheresrhys commented May 29, 2019

Could do. I'm actually looking for more people to help maintain the library in general too. I'm away a lot over the next few weeks. If you're on twitter send me a DM and we can work something out

I'm planning a v8 release soon anyway to replace babel-polyfill with transpile-runtime. Could an opportune moment to bump core-js too, though a solution that is core-js version agnostic would be even better.

@grug

This comment has been minimized.

Copy link
Collaborator

@grug grug commented May 30, 2019

Sweet - I've pinged you on Twitter :)

i-like-robots added a commit to Financial-Times/x-dash that referenced this issue Jun 7, 2019
…ch errors

Currently fetch-mock ships a transpiled copy of its code for use in the browser. This requires
several Babel runtime and Core JS helpers and polyfills. However the code is transpiled using
an older version of Babel which uses core-js v2 but everything we now use depends on core-js v3.

wheresrhys/fetch-mock#419
i-like-robots added a commit to Financial-Times/x-dash that referenced this issue Jun 7, 2019
…ch errors

Currently fetch-mock ships a transpiled copy of its code for use in the browser. This requires
several Babel runtime and Core JS helpers and polyfills. However the code is transpiled using
an older version of Babel which uses core-js v2 but everything we now use depends on core-js v3.

wheresrhys/fetch-mock#419
@travist

This comment has been minimized.

Copy link
Contributor

@travist travist commented Jun 11, 2019

We were able to solve this pretty easily by just forcing this library to have a dependency on core-js@2.

I know this seems counter-intuitive, but by making this library dependent on core-js@2, you are allowing dependent libraries to require core-js@3 and NPM will then install a "local" node_modules folder in this library and everything just starts to work. Here is the pull request that fixes it.

#432

@Dynalon

This comment has been minimized.

Copy link

@Dynalon Dynalon commented Jun 26, 2019

I'd side with @robertknight that you shouldn't directly import from core-js but rather leave it to the consumer to establish ES5 compat. Babel comes with babel-polyfill which is essentially just core-js under the hood, and angular also uses core-js. FBs create-react-app also comes with babel-polyfill out of the box.

Additionally, you shouldn't probably hard-code polyfills for any ES5 stuff anymore for version 8.0, as there are hardly any modern browsers requiring polyfills at all. Given that fetch-mock is a prototyping/testing tool, support for IE11 etc. is hardly a requirement. And if you absolutely need support for older browsers, you'll probably have babel+core-js present anyways in your build stack or you can fall back to using fetch-mock 7.x

@jquense

This comment has been minimized.

Copy link

@jquense jquense commented Jun 27, 2019

Please release a build without the polyfills!

@wheresrhys

This comment has been minimized.

Copy link
Owner

@wheresrhys wheresrhys commented Jun 27, 2019

Have released with @travist's fix

Please give it a go

@grug

This comment has been minimized.

Copy link
Collaborator

@grug grug commented Jun 28, 2019

@wheresrhys will this be published to NPM?

@CamBurris

This comment has been minimized.

Copy link

@CamBurris CamBurris commented Jul 9, 2019

@wheresrhys Can we get a publish of the latest release to npm? It seems npm is behind by two versions at this point. It would be nice to have this fix published.

@bjankord

This comment has been minimized.

Copy link

@bjankord bjankord commented Jul 9, 2019

I've tested a project that depends on core-js v3 and pulls in the fetch-mock v7.3.5 tarball and my webpack script ran successfully compiling the modules. Looking forward to seeing v7.3.5 released on npm.

@wheresrhys

This comment has been minimized.

Copy link
Owner

@wheresrhys wheresrhys commented Jul 9, 2019

Damn - broken build. Sorry for the delay - missed the earlier message

Fixed in 7.3.6

@andreiho

This comment has been minimized.

Copy link

@andreiho andreiho commented Jul 12, 2019

Somehow I'm stil experiencing this issue. I have core-js@3 installed in the root node_modules and core-js@2 installed locally in fetch-mock. I'm using TypeScript, not sure whether it makes a difference or not.

@cushind

This comment has been minimized.

Copy link

@cushind cushind commented Jul 12, 2019

I'm still experiencing this issue as well. Same deal as @andreiho but not using TypeScript.

@jdelStrother

This comment has been minimized.

Copy link

@jdelStrother jdelStrother commented Jul 16, 2019

Yeah, I'm still seeing the same problem as @andreiho & @cushind with 7.3.6. However... I only seem to get it when importing fetch-mock into a storybook build, rather than my regular webpack build. I'm still working on figuring out what the difference between the two setups is.

@andreiho

This comment has been minimized.

Copy link

@andreiho andreiho commented Jul 16, 2019

@jdelStrother Same actually, only with Storybook.

@jdelStrother

This comment has been minimized.

Copy link

@jdelStrother jdelStrother commented Jul 17, 2019

So Storybook uses corejs-upgrade-webpack-plugin which seems fairly broken - storybookjs/storybook#7445

In the meantime, I've hacked around it to remove that plugin in my storybook config. That looks something like this, in .storybook/webpack.config.js:

module.exports = function(options) {
  var config = options.config
  config.plugins = config.plugins.filter(
    p => String(p.resourceRegExp) !== "/core-js/"
  );
  return config
fenglish added a commit to Financial-Times/x-dash that referenced this issue Aug 22, 2019
…ch errors

Currently fetch-mock ships a transpiled copy of its code for use in the browser. This requires
several Babel runtime and Core JS helpers and polyfills. However the code is transpiled using
an older version of Babel which uses core-js v2 but everything we now use depends on core-js v3.

wheresrhys/fetch-mock#419
@wheresrhys

This comment has been minimized.

Copy link
Owner

@wheresrhys wheresrhys commented Oct 26, 2019

v8.0.0-alpha.2 attempts to address this issue. Please leave a 👍 on the pull request #457 if your problem is solved, or alternatively leave a comment @i-like-robots @emyarod @BenjaminVanRyseghem @dancrumb @steveoh @joerodrig @andreiho @cushind @jdelStrother @mkay581 @sahariko @bjankord @CamBurris @kajyr @Dynalon @travist @thommyhh @gfortaine @robertknight @poorpaddy @iamnewton

@wheresrhys

This comment has been minimized.

Copy link
Owner

@wheresrhys wheresrhys commented Oct 26, 2019

Closing in favour of discussion on the PR

@wheresrhys wheresrhys closed this Oct 26, 2019
@robertknight robertknight mentioned this issue Oct 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.