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

Prerequisites: Use system version of Node #1201

Merged
merged 1 commit into from
May 7, 2024

Conversation

stevepolitodesign
Copy link
Contributor

We were incorrectly assuming consumers had the version of Node installed that is declared in Suspenders::NODE_LTS_VERSION.

Because of this, we were generating a .node-version file with a version that was not installed on the user's system. This was a problem because when subsequent generators depending on Node where invoked, they would raise the following:

No preset version installed for command node
Please install a version by running one of the following:

asdf install nodejs 20.11.1

To account for this, we borrow from Rails, but modify slightly. First, we look to see if ENV["NODE_VERSION"] is set. If not, we then look to see if Node is installed, and set the version via node --version. If neither of those values are present, we raise. This is because we can't use a fallback value, since the consumer does not have Node installed on their system.

Removes Suspenders::NODE_LTS_VERSION since it's no longer used as a fallback.

Introduced climate_control as a development dependency in order to test this feature.

@stevepolitodesign stevepolitodesign requested a review from thiagoa as a code owner May 7, 2024 09:45
We were incorrectly assuming consumers had the version of Node installed
that is declared in `Suspenders::NODE_LTS_VERSION`.

Because of this, we were generating a `.node-version` file with a
version that was not installed on the user's system. This was a problem
because when subsequent generators depending on Node where invoked, they
would raise the following:

```
No preset version installed for command node
Please install a version by running one of the following:

asdf install nodejs 20.11.1
```

To account for this, we [borrow][] from Rails, but modify slightly.
First, we look to see if `ENV["NODE_VERSION"]` is set. If not, we then
look to see if Node is installed, and set the version via `node
--version`. If neither of those values are present, we raise. This is
because we can't use a fallback value, since the consumer does not have
Node installed on their system.

Removes `Suspenders::NODE_LTS_VERSION` since it's no longer used as a
fallback.

Introduced [climate_control][] as a development dependency in order to
test this feature.

[borrow]: https://github.com/rails/rails/blob/d65fec4fd2a047533408cbbd4824b248adc0e3fd/railties/lib/rails/generators/app_base.rb#L521-L529
[climate_control]: https://github.com/thoughtbot/climate_control
@stevepolitodesign stevepolitodesign force-pushed the suspenders-3-0-0-node-version branch from dd27fec to db14f14 Compare May 7, 2024 09:46
Comment on lines +22 to +24
def node_version
ENV["NODE_VERSION"] || `node --version`[/\d+\.\d+\.\d+/]
end
Copy link
Contributor Author

@stevepolitodesign stevepolitodesign May 7, 2024

Choose a reason for hiding this comment

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

As an aside, the code we lifted from Rails actually has a bug. This is why I modified it here to not use a fallback.

@stevepolitodesign stevepolitodesign merged commit 7bf6e82 into suspenders-3-0-0 May 7, 2024
2 checks passed
@stevepolitodesign stevepolitodesign deleted the suspenders-3-0-0-node-version branch May 7, 2024 09:50
Copy link
Contributor

@mike-burns mike-burns left a comment

Choose a reason for hiding this comment

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

Good fix.

@stevepolitodesign stevepolitodesign mentioned this pull request May 7, 2024
17 tasks
stevepolitodesign added a commit that referenced this pull request May 8, 2024
Follow-up to #1201

It's not enough to ensure Node is installed. We also need to ensure the
consumer has the supported minimum version installed. Otherwise,
subsequent generators will raise errors like so:

```
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=18.12.0". Got "18.0.0"
error Found incompatible module.
```

We select `v20.0.0` as our minimum supported version because it is
slated for Active LTS, but is not bleeding edge at [this time][]

[this time]: https://nodejs.org/en/about/previous-releases
stevepolitodesign added a commit that referenced this pull request May 8, 2024
Follow-up to #1201

It's not enough to ensure Node is installed. We also need to ensure the
consumer has the supported minimum version installed. Otherwise,
subsequent generators will raise errors like so:

```
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=18.12.0". Got "18.0.0"
error Found incompatible module.
```

We select `v20.0.0` as our minimum supported version because it is
slated for Active LTS, but is not bleeding edge at [this time][]

We also raise when calling the template to avoid unnecessarily
generating a new Rails application.

[this time]: https://nodejs.org/en/about/previous-releases
stevepolitodesign added a commit that referenced this pull request May 8, 2024
Follow-up to #1201

It's not enough to ensure Node is installed. We also need to ensure the
consumer has the supported minimum version installed. Otherwise,
subsequent generators will raise errors like so:

```
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=18.12.0". Got "18.0.0"
error Found incompatible module.
```

We select `v20.0.0` as our minimum supported version because it is
slated for Active LTS, but is not bleeding edge at [this time][]

We also raise when calling the template to avoid unnecessarily
generating a new Rails application.

[this time]: https://nodejs.org/en/about/previous-releases
stevepolitodesign added a commit that referenced this pull request May 8, 2024
Follow-up to #1201

It's not enough to ensure Node is installed. We also need to ensure the
consumer has the supported minimum version installed. Otherwise,
subsequent generators will raise errors like so:

```
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=18.12.0". Got "18.0.0"
error Found incompatible module.
```

We select `v20.0.0` as our minimum supported version because it is
slated for Active LTS, but is not bleeding edge at [this time][]

We also raise when calling the template to avoid unnecessarily
generating a new Rails application.

[this time]: https://nodejs.org/en/about/previous-releases
stevepolitodesign added a commit that referenced this pull request May 10, 2024
We were incorrectly assuming consumers had the version of Node installed
that is declared in `Suspenders::NODE_LTS_VERSION`.

Because of this, we were generating a `.node-version` file with a
version that was not installed on the user's system. This was a problem
because when subsequent generators depending on Node were invoked, they
would raise the following:

```
No preset version installed for command node
Please install a version by running one of the following:

asdf install nodejs 20.11.1
```

To account for this, we [borrow][] from Rails, but modify slightly.
First, we look to see if `ENV["NODE_VERSION"]` is set. If not, we then
look to see if Node is installed, and set the version via `node
--version`. If neither of those values are present, we raise. This is
because we can't use a fallback value, since the consumer does not have
Node installed on their system.

Removes `Suspenders::NODE_LTS_VERSION` since it's no longer used as a
fallback.

Introduced [climate_control][] as a development dependency in order to
test this feature.

[borrow]: https://github.com/rails/rails/blob/d65fec4fd2a047533408cbbd4824b248adc0e3fd/railties/lib/rails/generators/app_base.rb#L521-L529
[climate_control]: https://github.com/thoughtbot/climate_control
stevepolitodesign added a commit that referenced this pull request May 10, 2024
Follow-up to #1201

It's not enough to ensure Node is installed. We also need to ensure the
consumer has the supported minimum version installed. Otherwise,
subsequent generators will raise errors like so:

```
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=18.12.0". Got "18.0.0"
error Found incompatible module.
```

We select `v20.0.0` as our minimum supported version because it is
slated for Active LTS, but is not bleeding edge at [this time][]

We also raise when calling the template to avoid unnecessarily
generating a new Rails application.

[this time]: https://nodejs.org/en/about/previous-releases
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