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

V8 #457

Merged
merged 37 commits into from
Nov 12, 2019
Merged

V8 #457

merged 37 commits into from
Nov 12, 2019

Conversation

wheresrhys
Copy link
Owner

@wheresrhys wheresrhys commented Oct 26, 2019

This

  • implements ES modules
  • hopefully resolves problems with babel-core clashes
  • adds typescript types
  • removes the UNMATCHED and MATCHED constants, and no longer exports fetchMock as a property, as well as the default, export
  • only includes the whatwg-url polyfill in nodejs

Reviews of the new documentation much appreciated

Note - the latest release is 8.0.0-alpha.10

@coveralls
Copy link

coveralls commented Oct 26, 2019

Coverage Status

Coverage increased (+0.006%) to 97.118% when pulling 5c06377 on v8 into f4616e8 on master.

@robertknight
Copy link

Thank-you for your efforts @wheresrhys 🙂. It turns out that fdf9d61 solved my immediate problem as described in #419 by making browserify use a compatible version of core-js when bundling fetch-mock. browserify respects the "browser" field so importing "fetch-mock" Just Works although the output is suboptimal.

The result of browserify -r fetch-mock (using v8.0.0-alpha3) is ~900KB of JS which is not ideal as more code makes tests run slower (we use headless Chrome + Karma as the main environment). The biggest offender, based on looking output from disc, seems to be whatwg-url. Removing the whatwg-url and babel-polyfill imports from the version of the code on master drops the unminified bundle size from 910KB to 95KB. I can work around this but it would be great if it wasn't necessary.

@robertknight
Copy link

I'm not familiar enough with the handling of CJS + ESM modules in different node versions / bundlers to do much of a technical review of the new documentation here, but I appreciate the fact that it exists. As I mentioned above, in our toolchain require("fetch-mock") / import "fetch-mock" works out of the box.

@wheresrhys
Copy link
Owner Author

wheresrhys commented Oct 26, 2019

Thanks for the feedback @robertknight. Are you able to tell which entry point your build is picking up? I guess it's ./es5/client.js.

Interesting to find the impact of whatwg-url on bundle size - most of the difference in size you're seeing is down to it I think, but it's inclusion is not new, and I actually see a slight reduction in size in v8. Are you seeing the bundle size go up in comparison?

@wheresrhys
Copy link
Owner Author

wheresrhys commented Oct 26, 2019

Tweaked it so it only requires whatwg-url on the server. The browser property of package.json also points at modern esm code, not es5, which should help a little more. I'll release as 8.0.0-alpha. Would appreciate if you could give it another whirl

@LarsDenBakker
Copy link

Thanks, this does solve my issue. And the readme is good too.

@berstend
Copy link

berstend commented Oct 27, 2019

There seems to be a bug somewhere (8.0.0-alpha.5, node v13.0.1)

project/node_modules/fetch-mock/package.json

...
"main": "./cjs/server.js",
...

image

With "main": "./cjs/src/server.js" it works. :)

PPS:
In Typescript import fetchMock from 'fetch-mock' doesn't work (Cannot find module 'fetch-mock'.ts(2307)) whereas const fetchMock = require('fetch-mock') works, with above bugfix.

@wheresrhys
Copy link
Owner Author

wheresrhys commented Oct 27, 2019

@berstend that's strange. This bug did exist briefly - up to alpha.2 I think - but should be fixed by 4eddade. Locally I get everything copied into the root of /cjs - no idea why the npm prepublish step would be behaving differently in CI. I will need to brush up on my BASH to figure out why

Does import fetchMock from 'fetch-mock' work in typescript with v7?

@wheresrhys
Copy link
Owner Author

That bug with the cjs export is now fixed. No idea why it was broken - both myself and CI run bash, so it's really odd that the same command had different results.

@Dynalon
Copy link

Dynalon commented Oct 28, 2019

Just upgraded to 8.0.0-alpha.11 and it works with core-js@3 👍

One little note: MockOptions is not exported in the typescript typings, but should be (it was exported in the @types/fetch-mock typings. Aside from that, I had to change import * as fetchMock from "fetch-mock" into import fetchMock from "fetchMock" but then everything worked fine using the mocks 👍

@wheresrhys
Copy link
Owner Author

@Dynalon thanks for trying it out. I'd really appreciate a PR onto this branch to fix the typescript issue as I'm not really a typescript aficionado, and I pretty much copied the type defs from DefinitelyTyped unchanged, so not sure how I could have dropped an export

@Dynalon
Copy link

Dynalon commented Oct 28, 2019

@Dynalon thanks for trying it out. I'd really appreciate a PR onto this branch to fix the typescript issue as I'm not really a typescript aficionado, and I pretty much copied the type defs from DefinitelyTyped unchanged, so not sure how I could have dropped an export

Forget what I said, it is still exported. I got mixed up with the different kind of module import that v8 brings. Everything fine 👍

@MathieuPuech
Copy link

I tried to use the esm/client.mjs without success, there is an error from querystring. It seems this is a native module on node but it does not exists in browsers.

@wheresrhys
Copy link
Owner Author

Thanks @MathieuPuech - I will investigate. Can you tell me a bit more about your toolchain? What bundlers, transpilers etc are you using, and in which environment are your tests running

Copy link
Contributor

@evan-10e evan-10e left a comment

Choose a reason for hiding this comment

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

👍

@wheresrhys
Copy link
Owner Author

nudge @MathieuPuech

@MathieuPuech
Copy link

MathieuPuech commented Oct 30, 2019

I’m using it with es-dev-server
https://www.npmjs.com/package/es-dev-server
There is no bundling, transpiler only for node_modules path resolution

@LarsDenBakker
Copy link

Fyi es dev server uses https://www.npmjs.com/package/resolve for resolving modules

@wheresrhys
Copy link
Owner Author

Thanks both - I will try to recreate the issue and hopefully address it

…o v8

* 'master' of https://github.com/wheresrhys/fetch-mock:
  fix bug in multiple calls of delay
  Bump lodash from 4.17.10 to 4.17.15
  improve example's richness
  split options into sections
* 'v8' of https://github.com/wheresrhys/fetch-mock:
  remove MATCHED and UNMATCHED constants
  updated types to support delay option
  add new mock() without arguments behaviour to types
  stop using localTs - I can see why not now. Want max compat
  add body option to type defs
  local typescript
  I think I've done a types??
@wheresrhys
Copy link
Owner Author

@MathieuPuech that should be fixed now. It's annoying that es-dev-server prefers module to browser, even when browser points at a .mjs file. I may raise an issue

@MathieuPuech
Copy link

It's working when using import fetchMock from 'fetch-mock/esm/client.mjs' instead of import fetchMock from 'fetch-mock' because of the resolution.

es-dev-server should maybe not resolve with module package.json property. But I don't know who is right because module property is not documented in npm js package.json documentation.

@wheresrhys wheresrhys merged commit 4881edf into master Nov 12, 2019
@wheresrhys wheresrhys deleted the v8 branch April 11, 2020 22:26
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.

None yet

8 participants