Skip to content

Update nodejs #1810

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

Merged
merged 5 commits into from
Oct 25, 2022
Merged

Update nodejs #1810

merged 5 commits into from
Oct 25, 2022

Conversation

ylecornec
Copy link
Member

@ylecornec ylecornec commented Aug 22, 2022

This PR makes rules_haskell use node 16 as mentioned in issue #1742.

  • For bindists, rules_nodejs is updated so that node 16 is now the default.
  • For nix, nodejs-16_x is now used instead of the default nodejs.

rules_nodejs update.

The way to use rules_nodejs 5.0.0 with nix changed quite a bit:

  • The node from nix is still installed via the _nixpkgs_nodejs macro but was modified a bit because node is now expected at a specific path.
  • In addition, the name of the external repository containing node is expected to end with the os name. Because of this we declare the same repository multiple times under all the possible names here, but only the one corresponding to the current os will be used. (At first I defined a new repository rule to get the os_name and recover it with a load command in the WORKSPACE file, but declaring all the possible names seems simpler).
  • yarn_install now has a yarn attribute that can point to a nix installed yarn, which we install with _nixpkgs_yarn.
  • We must now declare the node toolchain ourselves (in the _declare_nix_node_toolchain_impl rule) and register it.
  • The name of the nodejs toolchain type changed to @rules_nodejs//nodejs:toolchain_type.

shellcheck

I encountered some shellcheck errors, mostly because it was trying to analyze template files which are not complete yet, so I added directives to these errors.

The --experimental-wasm-bigintoption

This option is not needed anymore with node 16, so the asterius bundle was updated to a new version to fix this (as mentioned in #1742).

@ylecornec ylecornec force-pushed the ylecornec/update_rules_nodejs branch from 6c64575 to 0e585ba Compare August 24, 2022 13:38
@ylecornec ylecornec marked this pull request as ready for review August 25, 2022 06:05
@ylecornec ylecornec force-pushed the ylecornec/update_rules_nodejs branch from 0e585ba to c793fe8 Compare August 25, 2022 06:44
@ylecornec ylecornec requested review from avdv and benradf August 29, 2022 11:30
Copy link
Member

@avdv avdv left a comment

Choose a reason for hiding this comment

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

LGTM 💯

Could you just add an entry to the Changelog.md file, please?

Copy link
Member

@avdv avdv left a comment

Choose a reason for hiding this comment

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

Thank you!

@avdv avdv added the merge-queue merge on green CI label Oct 25, 2022
@mergify mergify bot merged commit 2c4e77c into master Oct 25, 2022
@mergify mergify bot deleted the ylecornec/update_rules_nodejs branch October 25, 2022 15:00
@mergify mergify bot removed the merge-queue merge on green CI label Oct 25, 2022
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.

2 participants