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

Require Node.js 12.20.0 and move to ESM #1141

Merged
merged 47 commits into from Jul 18, 2021
Merged

Require Node.js 12.20.0 and move to ESM #1141

merged 47 commits into from Jul 18, 2021

Conversation

xxczaki
Copy link
Member

@xxczaki xxczaki commented May 3, 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)

  • Converted the module to pure ESM (the minimal Node.js version is now 12.8.0)
  • Updated all dependencies & devDependencies
  • Removed rollup and commonjs-related things (mainly tests)
  • Linted everything with the newest version of xo
  • Updated readme, changelog and funding.yml.
  • Removed commonjs GitHub action

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

#668

Is there anything you'd like reviewers to know?

Reference material: https://blog.sindresorhus.com/get-ready-for-esm-aa53530b3f77

@xxczaki xxczaki mentioned this pull request May 3, 2021
@types/index.test-d.ts Outdated Show resolved Hide resolved
example.js Outdated Show resolved Hide resolved
@Richienb Richienb changed the title Require Node.js >=12.8 & transition to pure ESM Require Node.js 12.8 and move to ESM May 5, 2021
@tinovyatkin
Copy link
Member

@xxczaki
This logic should also be adjusted

LinusU and others added 4 commits July 16, 2021 09:42
* Test on all minimum supported Node.js versions

* Tweak Node.js workaround version range

* Handle Node.js 16 aborted error message

* fix node version string compare

Co-authored-by: Jimmy Wärting <jimmy@warting.se>
@jimmywarting
Copy link
Collaborator

@Richienb, @LinusU could you review this?
looks finish

README.md Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jimmywarting jimmywarting changed the title Require Node.js 12.8 and move to ESM Require Node.js 12.20.0 and move to ESM Jul 16, 2021
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.

Great work everyone! 🚀

package.json Show resolved Hide resolved
test/main.js Show resolved Hide resolved
test/main.js Outdated Show resolved Hide resolved
@jimmywarting
Copy link
Collaborator

jimmywarting commented Jul 17, 2021

@gr2m

Did you test that the import of node-fetch is working for you? I'll try to give it a try myself

No i haven't tested node-fetch but i have tried out formdata-polyfill and fetch-blob that is a ESM only pacakge. They are ESM only package and only have a main

the fetch-blob package have stuff like: fetch-blob/from.js and fetch-blob/file.js
formdata-package has formdata-polyfill/esm.min.js, formdata-polyfill/formdata-to-blob.js that you can import/use aswell


Small side note: don't really like the look of this import mapping: "./response": "./src/response.js"
Just so you can get rid of src
NodeJS is adding support for http loader so you can load a package without using NPM, think it would be missleading if we recommend (in the readme) that you can use node-fetch/response when you sould actually be using .../node-fetch/src/response.js when using a http loader instead.
Browser and Deno do not have support for NPM packages, they can only load other packages using urls. so you should not keep changing filename and moving files around much...

so my reasoning is: don't mess with exports property in package.json if you are going to support loading a package/module that can work for both Browser, Deno & NodeJS using http loader

Alldoe it could be a good idea to do something like "exports": "src/*"

@gr2m
Copy link
Collaborator

gr2m commented Jul 18, 2021

I won't die on my exports with explicitly defined exports hill 😁 If you feel strong about leaving main let's do that and see how it goes. It's a problem when it's a problem, we shall find out.

@jimmywarting
Copy link
Collaborator

I don't have so strong feelings for it either. (even doe it might sound like it)
but yea, "It's a problem when it's a problem" then we will fix it

@jimmywarting jimmywarting merged commit b50fbc1 into master Jul 18, 2021
@jimmywarting jimmywarting deleted the esm branch July 18, 2021 20:15
ImRodry added a commit to Hypixel-Translators/hypixel-translators-bot that referenced this pull request Sep 1, 2021
ImRodry added a commit to Hypixel-Translators/hypixel-translators-bot that referenced this pull request Sep 1, 2021
@Jiralite Jiralite mentioned this pull request Nov 5, 2021
4 tasks
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

9 participants