Skip to content

Conversation

legobeat
Copy link
Collaborator

@legobeat legobeat commented Feb 21, 2024

  • Base workflow broken out from fix: Buffer warning & update ci #24
  • ci: Remove Travis CI configuration
  • ci: Add GitHub Actions workflow testing under node majors 0.10~8
  • chore: Remove test-all package script

Note: Modern node version tests fail but this is expected and should be observable if running in Travis or locally as well. The failing node-versions (>=v10) have been disabled for the time being. Not sure if it's worth effort to test 0.6 and 0.8 at this point?

@legobeat legobeat marked this pull request as ready for review February 21, 2024 08:54
Copy link
Owner

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

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

I'd like to get this in before publishing a new version.
Can you adapt this to pass for now?

Then we publish a patch and afterwards merge the one upgrading node versions (we can drop all below v14.x (given 16 is running out of maintenance).

@legobeat
Copy link
Collaborator Author

legobeat commented Feb 22, 2024

Can you adapt this to pass for now?

In order to not make this grow as large as the original #24 , what I did here for now is to disable CI tests on Node >=v10 (which in any case isn't a regression compared to the existing travis config, which never tested that high).

IIUC, to make tests properly pass consistently across all currently supported node majors, the sourcemap tests would have to be rewritten to switch expected result depending on major node version (cf https://github.com/thlorenz/inline-source-map/pull/28/files#diff-4a16505372866dba552bc5422181172bacaef66a1c47df075590ac64f1eea4f4L51). Doing that for the sake of transiently passing these ancient node versions doesn't seem worth the effort.

zichang-nong and others added 2 commits February 22, 2024 05:18
- ci: Remove Travis CI configuration
- ci: Add GitHub Actions workflow testing under node majors 4~20
- chore: Remove `test-all` package script
@thlorenz
Copy link
Owner

OK gonna merge this, I saw you disabled newer node versions which should make CI pass right?
One last favor, please update the badge in the readme to show gha status.

Also I see those actions are running in your fork, but they aren't here even though I got those enabled. Any idea what I need to do to run those for this PR before I merge it?

@thlorenz
Copy link
Owner

Also thanks for your patience, the main reason I'd like CI to pass is that we're planning to publish a patch for before upgrading Node version requirements and I feel bad doing that with failing CI.

@legobeat
Copy link
Collaborator Author

legobeat commented Feb 23, 2024

Also thanks for your patience, the main reason I'd like CI to pass is that we're planning to publish a patch for before upgrading Node version requirements and I feel bad doing that with failing CI.

All good!

Actually it turned out that the Buffer fix in #25 did break compatibility with Node versions < 4. So merging this as-is will make those versions fail (which I personally would think is fine transiently, it highlights what's going on and will be resolved when fixed).

@legobeat
Copy link
Collaborator Author

legobeat commented Feb 23, 2024

OK gonna merge this, I saw you disabled newer node versions which should make CI pass right?

Only one caveat: See comment above.

One last favor, please update the badge in the readme to show gha status.

Badge in readme has been replaced!

preview (that's with #29 applied as well)

Also I see those actions are running in your fork, but they aren't here even though I got those enabled. Any idea what I need to do to run those for this PR before I merge it?

The one thing you would need to do would be to enable actions on repo-level, I believe.

https://github.com/thlorenz/inline-source-map/settings/actions -> Actions permissions

@thlorenz thlorenz merged commit 6f7aa70 into thlorenz:master Feb 24, 2024
@legobeat legobeat mentioned this pull request Mar 13, 2024
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.

3 participants