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

fix: import whatwg-url in a way compatible with ESM Node #1303

Merged
merged 2 commits into from Sep 22, 2021

Conversation

LinusU
Copy link
Member

@LinusU LinusU commented Sep 21, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New feature
  • Other, please explain:

What changes did you make? (provide an overview)

I imported whatwg-url in a way that is compatible with Node.js running in ESM mode.

Which issue (if any) does this pull request address?

ace7536#r56836783

Is there anything you'd like reviewers to know?

n/a

closes #1305

src/request.js Show resolved Hide resolved
@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 21, 2021

Any possible way to unit test this also?
changelog + patch version also?

@LinusU
Copy link
Member Author

@LinusU LinusU commented Sep 21, 2021

Any possible way to unit test this also?

I haven't found one yet, since I cannot trigger the issue myself (even though I think that you shouldn't be able to import it like that 😄)

Waiting for more information from @robrecord

changelog + patch version also?

Will fix

@robrecord
Copy link

@robrecord robrecord commented Sep 21, 2021

Thank you, I will review ASAP.

@robrecord
Copy link

@robrecord robrecord commented Sep 21, 2021

I have confirmed this fixes the error I was facing. Thank you!

@paranoidjk
Copy link

@paranoidjk paranoidjk commented Sep 22, 2021

This should fix #1305

@LinusU
Copy link
Member Author

@LinusU LinusU commented Sep 22, 2021

@robrecord or @paranoidjk do you know of any easy way we could add a test for this?

@LinusU
Copy link
Member Author

@LinusU LinusU commented Sep 22, 2021

@jimmywarting I've added changelog entry and bumped the package version. What do you think of merging this without a test for now, and adding one later as this seems to be breaking some peoples workflow and they have to pin node-fetch at a lower version currently?

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 22, 2021

Yea, can release it without a test

@jimmywarting jimmywarting merged commit b5417ae into 2.x Sep 22, 2021
@jimmywarting jimmywarting deleted the 2.x-import-mjs-fix branch Sep 22, 2021
@robrecord
Copy link

@robrecord robrecord commented Sep 22, 2021

If I can find an easy way to have this tested I will add it here.

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 22, 2021

If I can find an easy way to have this tested I will add it here.

Great

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