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

Correct engine requirements #419

Merged
merged 1 commit into from
May 30, 2024
Merged

Conversation

fdc-viktor-luft
Copy link
Collaborator

@fdc-viktor-luft fdc-viktor-luft commented May 25, 2024

Is the travis.yml still used or can it be completely removed?

@kamiazya
Copy link
Collaborator

@fdc-viktor-luft Hi,

If the updated engine requirement is accurate, please consider updating madge to use version 2 of the ts-graphviz package, which I maintain.

Version 2 of ts-graphviz supports Node.js 18 and above. I had not submitted a patch earlier because I believed madge was still supporting Node.js 14.

Since version 1 of ts-graphviz will no longer receive patches, I recommend switching to version 2 at this time.

@kamiazya
Copy link
Collaborator

I tested it locally, and it seems that only the change to package.json is needed since there are no API changes!

$ node -v
v18.20.3

# fix package.json
#   
# “ts-graphviz”:“^2.1.2”.

$ npm i
...

$ node bin/cli.js -i /dev/null bin/cli.js 
Processed 8 files (878ms) (1 warning)

✔ Image created at /dev/null

@fdc-viktor-luft
Copy link
Collaborator Author

I'll include the update. Thanks for pointing this out. Since an update to the dependency dependency-tree was already merged that has this requirement, adjusting the engine requirement was kind of just missing.

I planned several other dependency updates as well.

But first I'd like to merge this one: #418
This includes a cleanup of all deps.

Then I'll do the recommended update of ts-graphviz. Thanks for your testing and the hint.

@PabloLION
Copy link
Collaborator

PR description

I'm just back to open-source world. I see that the description is not very clear, here's the change I suggest

Update Node.js version requirements and testing configurations.

- Increase the minimum required Node.js version from ">=14" to ">=18" in `package.json` to ensure compatibility with newer features and security updates.
- Update the Node.js version in `.nvmrc` from "18" to "20", aligning the development environment with the latest stable release.
- Adjust `.travis.yml` to test against Node.js versions "18" and "20", removing version "14" from the testing matrix to reflect the updated version requirements and focus on current and future Node.js versions.

Another thing

I recommend switching to version 2 at this time.

Maybe we can do it in another PR?

Off-topic

Today I tried the Copilot Workspace session. I assume this tool can help relief the workload. You guys can try it too.

@pahen pahen self-requested a review May 30, 2024 09:35
@pahen
Copy link
Owner

pahen commented May 30, 2024

Is the travis.yml still used or can it be completely removed?

It can be removed. But should maybe be done in another PR since everything related to Travis should be removed, including the build status badge in the README.

@fdc-viktor-luft fdc-viktor-luft merged commit af88f3a into pahen:master May 30, 2024
2 checks passed
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.

4 participants