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: handle bom in text and json #1739

Merged
merged 2 commits into from May 8, 2023
Merged

Conversation

aclarembeau
Copy link

@aclarembeau aclarembeau commented May 1, 2023

Purpose

Bring the fix https://github.com/node-fetch/node-fetch/pull/1482/commits from 3.X to 2.X

Changes

Allow parsing results with BOM encoding

Additional information

It's just a copy paste of the fix of https://github.com/node-fetch/node-fetch/pull/1482/commits for node-fetch 2.X, for the people who can't use 3.X


  • I updated readme
  • I added unit test(s)

  • fix #000

@smeet666
Copy link

smeet666 commented May 5, 2023

I would really love to see this change accepted, I'm in the exact same case where I can't upgrade to 3.0 now but I don't like too much to patch it on my side.

@jimmywarting
Copy link
Collaborator

jimmywarting commented May 5, 2023

Normally the process have been that at least 2 core members needs to review and accept, if at least 2 agrees then we will merge.
But if it may take to long or nobody have time to review then 1 can be enough.

we also don't do so much in the v2 branch anymore unless it's a security issue.

ping @LinusU


I can't upgrade to 3.0

any particular reason why you can't update?
there isn't any problem loading ESM module from CJS
you just have to load it lazily with import('node-fetch').then(...) read more here of how you can load v3

Another suggestion is just to update your nodejs version to v18/20 that have fetch built in, so no dependencies hare needed.

v20 is required if you want to replace the ways you get blob/files backed up by the file system. for instance: fileFromAsync(path) -> fs.openAsBlob(path)

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Neat! 👍

@jimmywarting jimmywarting merged commit 29909d7 into node-fetch:2.x May 8, 2023
@github-actions
Copy link

github-actions bot commented May 8, 2023

🎉 This PR is included in version 2.6.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jon-esperanza
Copy link

jon-esperanza commented May 8, 2023

This has deprecated Node.js 10 for this node-fetch version due to TextDecoder being used on the global object.

TextDecoder became available on the global object in Node.js 11. Shouldn't this be a breaking change? Is this allowed for 2.x?
Screenshot 2023-05-08 at 4 44 44 PM

jimmywarting added a commit that referenced this pull request May 9, 2023
@LinusU
Copy link
Member

LinusU commented May 9, 2023

@jon-esperanza the breakage was not intentional, we are reverting this now. Thank you for bringing this to our attention.

revert progress: #1741

jimmywarting added a commit that referenced this pull request May 9, 2023
@smeet666
Copy link

smeet666 commented Jun 1, 2023

Is there any status on this one, then? Is it abandoned? It's marked as merged, but seems to be reverted.

@jimmywarting
Copy link
Collaborator

Yes. it's reverted b/c it broke backward comp.
TextDecoder was not available in those older NodeJS

Must come up with a own solution that strips BOM manually without using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants