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

Branch nodev20 support #80

Merged
merged 4 commits into from
Nov 3, 2023
Merged

Branch nodev20 support #80

merged 4 commits into from
Nov 3, 2023

Conversation

paulo-ocean
Copy link
Contributor

@paulo-ocean paulo-ocean commented Oct 31, 2023

Fixes #79 .

Changes proposed in this PR:

  • update for node 20, v20 LTS set on .nvmrc
  • Update some proj configs
  • Do relative imports with file extension to avoid node modules conflicts/issues

@paulo-ocean
Copy link
Contributor Author

btw, its better if you have 'nvm' already installed, since there is the.nvmrc file, we can just nvm use

Copy link
Member

@bogdanfazakas bogdanfazakas left a comment

Choose a reason for hiding this comment

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

LGTM

Maybe worth pulling CODEOWNERS commit from main and adding more handles so we all get notifications when PR's are ready fro review

src/index.ts Show resolved Hide resolved
Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

Tests are still using node 16 :)

@paulo-ocean paulo-ocean requested a review from alexcos20 November 2, 2023 15:52
@an7e
Copy link
Contributor

an7e commented Nov 2, 2023

@paulo-ocean Here's a solution, but it requires an additional parameter in Node startup and most likely @alexcos20 will not approve this option

https://github.com/nodejs/loaders-test/tree/main/commonjs-extension-resolution-loader
there are other loaders in this repository if interested

@paulo-ocean
Copy link
Contributor Author

paulo-ocean commented Nov 2, 2023

@paulo-ocean Here's a solution, but it requires an additional parameter in Node startup and most likely @alexcos20 will not approve this option

https://github.com/nodejs/loaders-test/tree/main/commonjs-extension-resolution-loader there are other loaders in this repository if interested

thanks @an7e
IMO that is just another workaround, as we're still trying to load ES modules as CommonJS, while the recommendations and the standards are going on the opposite direction. The file extensions are required on Nodejs,
https://nodejs.org/api/esm.html#mandatory-file-extensions
anything else is just adding sugar IMO

@paulo-ocean
Copy link
Contributor Author

Tests are still using node 16 :)

I've already updated it, but PR is still blocked

@paulo-ocean paulo-ocean merged commit f65f0b2 into develop Nov 3, 2023
4 checks passed
@alexcos20 alexcos20 deleted the branch-nodev20-support branch January 19, 2024 10:59
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.

5 participants