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

fix: dependency updates in preparation for adding typescript #206

Open
wants to merge 3 commits into
base: v2
Choose a base branch
from

Conversation

wayfarer3130
Copy link

Just some changes for v2 to start updating versions/packaging for changes to typescript.

@pimterry
Copy link
Owner

pimterry commented Mar 6, 2025

Browser tests were failing (here and in the other PR) due to GitHub changing ubuntu-latest to Ubuntu-24.04, which apparently requires some different setup for Chrome, and broke the browser tests. I've pinned that bit of the job to 22.04 for now, which should resolve this for now (for a couple of years at least) and so the v2 branch now passes correctly. If you rebase onto the latest v2 branch that will resolve that issue.

Looks like something else in this PR is breaking the other jobs though, so that will need investigation.

I see you've pulled out the dropping-dist change into this PR. I could be persuaded either way on this, let's see what @Mr0grog thinks, I'm happy to go with whatever you two agree on.

@Mr0grog
Copy link
Contributor

Mr0grog commented Mar 9, 2025

Looks like something else in this PR is breaking the other jobs though, so that will need investigation.

Ahhh, yeah, back in #190, #191, and #195, there were some reasonably complex interdependencies between a lot of the dev dependencies. You can’t just update them all to latest; they do not all work together in all versions or all supported Node.js versions. This might be a bit complex to unravel.

I’m almost certain some of these upgrades will also require removing some Node.js versions from the test matrix. They just aren’t compatible with Node.js versions as old as we currently run tests in.

(Back when I was making those dev updates, I did not upgrade everything to the latest possible because I was trying to keep the divergence between dev and runtime Node.js support as small as possible. We eventually agreed they don’t have to stay perfectly aligned, and that skew will just have to become larger with a lot the changes we’re talking about for 2.0. But it will always present complexities, I think, since the compatibility goals of this library, even in 2.0, are just way wider than what most of the JS community is targeting these days.)

I see you've pulled out the dropping-dist change into this PR. I could be persuaded either way on this, let's see what @Mr0grog thinks, I'm happy to go with whatever you two agree on.

I am somewhat ambivalent here. I think it’s nice that previous versions have been easily downloadable directly from raw.github.com, and that will end with this change. But there are other ways — it could be included as an asset on releases in GitHub. I think this is fine as long as we are planning something like that. This should at least remove discussion of downloading from dist in the README, though.

@wayfarer3130
Copy link
Author

There is a big difference between libraries required for building and those required for running. I agree that for running it should run a long ways back on node versions. For building, however, (devDependencies), it should probably use a fairly recent node version with targetting for much older versions.
If the testing itself requires older versions of libraries for testing on older platforms, then probably having a sub-module to setup tests specifically for older versions is required in order to test both new and older versions of the libraries themselves. That older test would specifically use the already generated dist file and not try to include the src version. The newer version would specifically use the newer library version. Maybe even using exports in the package to allow generating a couple of different dist versions depending on exactly what is being imported. The existing min file would great for a CDN, but then a newer ESM version could also be provided and included as loglevel/esm. That would also allow including the minified version as loglevel/min

@Mr0grog
Copy link
Contributor

Mr0grog commented Mar 10, 2025

Sure! No argument from me. Lots of ways you could solve these issues. I’m not a maintainer here (just a past contributor in this area), so I can’t tell you what approaches are OK. I’ve generally tried to keep it minimal and conservative in the past, but that has obviously left us with a weird dependency configuration, and created some snags you might be tripping over here.

@pimterry commented “Looks like something else in this PR is breaking.” I was just trying to help point towards likely causes (not suggest how you needed to fix):

  1. Some of the upgrades you’re making here will require removing some Node.js versions from the test matrix. For example, Jasmine 5 requires Node.js 18+, so you’ll need to remove 10, 12, 14, and 16 from the test matrix. Or you’ll need to rewrite tests to run in a wider variety of environments and not depend on Jasmine.

  2. A year ago, I made some narrow updates for security and to get builds/tests working on current Node.js versions. My recollection was that the latest version of some dependencies does not work with the latest version of some other dependencies. Maybe that’s changed! This was a year ago. But that might also be part of the problem.

Lots of ways you could address both of these, including creating separate environments and/or dependency configurations for different dev-time tasks, as you noted.

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