Upgrade test related dependencies & adapt tests

civiccc / react-waypoint

A React component to execute a function whenever you scroll to an element.

MIT License

4.08k stars 208 forks source link

Upgrade test related dependencies & adapt tests #342

Closed realityking closed 3 years ago

realityking commented 3 years ago

This one was interesting as quite a few things broke when upgrading stuff.

Karma to version 6 worked as-is.
jasmine-core to 2.99 was a relatively simple change. I believe the test has never worked, jasmine-core 2.7 just exposed that
On that note, I couldn't figure out how to get up to jasmine-core 3
Webpack 3 just worked but Webpack 4 required some changes as Webpack < 4 didn't correctly execute code in strict mode.

I think most of these changes are no brainers except perhaps the upgrade to webpack. I think it's valuable to test with older webpack versions to assure compatibility. However webpack 4 is now almost 3 years old. I think it's fine to ignore these older versions. Especially since it uncovered tests weren't actually executed in strict mode, potentially hiding bugs.

trotzig commented 3 years ago

Thanks -- looks good to me!

The test failure in Node v15 is due to a change in node how peer dependencies are handled. I think this could be fixed by adding react-dom as a peer dependency (which I think should already have been made). You could also use npm install --legacy-peer-deps as a workaround (although I'm not sure how to configure that for Travis).

realityking commented 3 years ago

Yeah npm 7 is an pain in the ass. I wonder how tests passed on master - none of the changes in either PR really touched the React dependency. I'll have a look what can be done.

What's your take on the webpack question? Good to go with webpack 4?

trotzig commented 3 years ago

I think what happened is that the node version named "node" (!) on Travis got updated from 14 to 15 at some point.

Yep, webpack 4 is fine. 👍

realityking commented 3 years ago

I thought so too but the last passing build already run with npm 7.0.8: https://travis-ci.org/github/civiccc/react-waypoint/jobs/743936673#L196

realityking commented 3 years ago

Quickly tested locally

node v15.4.0 (npm v7.0.15): works ✅
node v15.5.1 (npm v7.3.0): breaks 🛑

Deviously, it works when the node_modules folder already exists but it breaks when doing a clean install. I think this might be an npm bug.

realityking commented 3 years ago

The issue has been already been reported to npm: https://github.com/npm/cli/issues/2514

I see two possible ways out of this until this is fixed:

Force --legacy-peer-deps for npm
Pin the node version to 15.4 and thus npm to 7.0

Which one do you prefer?

realityking commented 3 years ago

@trotzig The npm bug has been fixed and CI passes. This is good to go from my perspective.

realityking commented 3 years ago

@trotzig It'd be great if you could have a look at this one :)