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

feat: add volta as node-version-file #532

Merged
merged 21 commits into from Sep 12, 2022
Merged

feat: add volta as node-version-file #532

merged 21 commits into from Sep 12, 2022

Conversation

jef
Copy link
Contributor

@jef jef commented Jun 29, 2022

Description:

Unlike other version managers, Volta places its versioning within package.json. We will check for volta.node to see if there is an value and use engines.node as backup. This is an important distinction, as volta.node should supersede engines.node.

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@jef jef requested a review from a team June 29, 2022 19:24
@dmitry-shibanov
Copy link
Contributor

Hello @jef. Thank you for your pull request. Could you please run the npm run format command ? Besides, do we need to declare package.json as possible input ?

@jef
Copy link
Contributor Author

jef commented Jun 29, 2022

Besides, do we need to declare package.json as possible input

thanks @dmitry-shibanov. great question. i also wasn't sure about this. i'm not sure if other projects use package.json for storing node.js versions. if so, then perhaps this could be more extendable.

edit: updating now to be a little more extendable. thanks for the input!

@dmitry-shibanov
Copy link
Contributor

Thank you for reply @jef. I think we can remove package.json from this pull request because we have pull requests with adding support for package.json engine.

@jef
Copy link
Contributor Author

jef commented Jun 29, 2022

pull requests with adding support for package.json engine.

ah, yes. i see that in #485. in this case, i believe there need some logic update into include volta.

should i wait until #485 is merged and then i proceed with mine?

@vsafonkin
Copy link

Hi @jef, could you please update your branch from main and resolve conflicts?

@jef
Copy link
Contributor Author

jef commented Jul 20, 2022

Sure thing. I'll be back from vacation next week.

@jef
Copy link
Contributor Author

jef commented Jul 27, 2022

Should be all good now @vsafonkin. Thanks!

@jef
Copy link
Contributor Author

jef commented Jul 28, 2022

I've realized that the documentation for package.json is missing. Should I add it in here?

@dmitry-shibanov
Copy link
Contributor

Hello @jef. Thank you for your suggestion. We would appreciate if you add this to the documentation, but if you want, we will do it on our side. Thank you!

@jef
Copy link
Contributor Author

jef commented Aug 3, 2022

Alright, I've added in the documentation! Everything seems good from my end. Let me know if you need anything extra.

@vsafonkin
Copy link

Hi @jef, you need to resolve conflicts according #553 Thank you in advance!

@jef
Copy link
Contributor Author

jef commented Aug 4, 2022

Hi @jef, you need to resolve conflicts according #553 Thank you in advance!

Not a problem! Should be all sorted. I also removed the try/catch on the latter half of parseNodeVersionFile only because it was throwing an error locally. Seemed better this way. I can revert if necessary.

Thanks!

@jef
Copy link
Contributor Author

jef commented Aug 17, 2022

@vsafonkin, anything else you need before getting this in? Thanks!

@dmitry-shibanov
Copy link
Contributor

Hello @jef. Sorry for the late response. Could you please e2e tests for volta ?

@e-korolevskii
Copy link
Contributor

Hi @jef!

Just a gentle ping - we would like to know if you are planning to implement e2e tests here.

Thank you in advance!

@jef
Copy link
Contributor Author

jef commented Aug 30, 2022

Sorry about that. Yes, I can implement some e2e tests. Is that going to be outside of the __tests__/installer.test.ts file?

@e-korolevskii
Copy link
Contributor

Hello @jef!

Thanks for the quick response!
According to our contributor's guide, end-to-end tests are in the workflows folder, particularly in the versions.yml file.

@jef
Copy link
Contributor Author

jef commented Sep 8, 2022

Hi @e-korolevskii, I believe I implemented something that should work for our needs. I can also combine it with the previous step if that makes more sense.

Thanks!

@e-korolevskii
Copy link
Contributor

Hi @jef!

Now it looks good and I think we are ready to merge it.
I just wanted to thank you again for your contribution!

@jef
Copy link
Contributor Author

jef commented Sep 9, 2022

Thanks for walking me through @e-korolevskii 😊

@marko-zivic-93 marko-zivic-93 merged commit 9f3a02b into actions:main Sep 12, 2022
deining pushed a commit to deining/setup-node that referenced this pull request Nov 9, 2023
…ctions#532)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 6.2.1 to 6.3.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v6.3.0/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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

8 participants