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

Changing to CommonJS -- simple cases #649

Closed
wants to merge 7 commits into from

Conversation

dmitriz
Copy link

@dmitriz dmitriz commented Jun 7, 2019

As suggested in #643
Single export cases are treated.

@dmitriz dmitriz mentioned this pull request Jun 7, 2019
@codecov-io
Copy link

codecov-io commented Jun 7, 2019

Codecov Report

Merging #649 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #649   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      7           
  Lines         587    582    -5     
  Branches      183    181    -2     
=====================================
- Hits          587    582    -5
Impacted Files Coverage Δ
src/response.js 100% <100%> (ø) ⬆️
src/abort-error.js 100% <100%> (ø) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/body.js 100% <100%> (ø) ⬆️
src/fetch-error.js 100% <100%> (ø) ⬆️
src/headers.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95286f5...b0d400b. Read the comment docs.

@bitinn
Copy link
Collaborator

bitinn commented Jun 9, 2019

This is a good direction I might consider, but can't merge at the moment due to various implications.

Also changes will have to be made on v3 branch. Master branch is v2 only for now.

@dmitriz
Copy link
Author

dmitriz commented Jun 9, 2019

This is a good direction I might consider, but can't merge at the moment due to various implications.

Sounds mysterious. I've thought the project was looking for new contributions... did I misunderstand?

Also changes will have to be made on v3 branch. Master branch is v2 only for now.

Any reasoning behind? The change is not breaking, not even a change in functionality.

@bitinn
Copy link
Collaborator

bitinn commented Jun 9, 2019

Sounds mysterious.

TL;DR: I can't merge it as the actual export isn't a part of the tests. I can't verify this to work in the wild. (which users can import using named export or just commonjs require).

And if the goal is to remove babel, then there is more work to check it actually works and provide a migration plan for cases that doesn't.

I do agree 2.x was doing too much and hence making our work more difficult, so we aim to rectify this in v3.

@bitinn
Copy link
Collaborator

bitinn commented Jun 9, 2019

Here is my suggestion to move forward:

  • Make the same PR against v3 branch.
  • Try to test different ways to import node-fetch if you can.
  • After we merged this and [WIP] Update dependencies #640, we can publish v3 alpha and try to iterate further on this.

(I would love to only deal with commonjs export, but v2 is not the place to do it.)

@Richienb
Copy link
Member

Richienb commented Sep 8, 2019

In the new roadmap, we are now replacing Babel and Rollup with Microbundle in d68d0ef.

@Richienb Richienb closed this Sep 8, 2019
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

4 participants