search

paralleldrive / riteway

Simple, readable, helpful unit tests.
MIT License
1.15k stars 35 forks source link

Maintenance: package updates. Mostly minor updates (babel+plugins, e… #331

Closed bkyoung closed 2 years ago

bkyoung commented 2 years ago

…slint+plugins, cheerio), but tape went from 5.2.x -> 5.6.x and React went from 17.0.x -> 18.2.x.

bkyoung commented 2 years ago

I'm merely offering this in case you are interested. I realize it may warrant more than a patch version bump due to react 18 being brought in as a dependency because it forces a hard line for riteway in terms of react compatibility/support. I don't know of a good way to make packages like this just assume whatever version is available if one is already installed in a project, which is what would be ideal, since riteway doesn't actually seem to care what version is installed as long as it's compatible with the code you're testing.

I did confirm this incarnation of riteway behaves correctly with react v18 on a project that was previously having issues because the project was developed against v18 and riteway was testing with v17. And I confirmed all the riteway tests still pass after making all the package updates.

bkyoung commented 2 years ago

According to https://docs.npmjs.com/cli/v8/configuring-npm/package-json it is possible to do several things:

  1. Set the version of react to "" or "*" (they are equivalent). This will cause riteway's react requirement to be satisfied if some version of react is already installed when riteway is installed, but will install the newest version if react is not yet installed.
  2. Add a section to package.json called peerDependencies and move react to this section, perhaps even making it an optional dependency in this section. Riteway is general-purpose enough that react is not a strict dependency for it to be useful. In fact, I used it for several days, writing tests against my redux reducers without issue in a project that had react 18 installed. It wasn't until I started trying to test react components that I noticed any problems (ultimately due to the mismatch in react versions). This could be a decent option, but is less ergonomic than option 1. It really comes down to where you land on the debate over out-of-the-box turn-key developer experience vs granular-control of all aspects of dependency management. I personally feel option 1 strikes a reasonable balance and favors dx, which is a more effective pursuit.
  3. Add a section to package.json called optionalDependencies and move react to this section. This option seems to offer the least. It will still try to install react, but won't fail if react ultimately fails to be installed on the system. Just eww. Hacktastic.

Since I am not familiar with the original design goals, I am certainly not in a position to assert any of these options are even aligned, however, from observation and use, it does appear that the purpose of the react dependency for riteway is to test target code, which increasingly is not the version of react on which riteway depends explicitly. Furthermore, it appears that riteway is invoking react to render code being tested, and actually doesn't depend on a specific version (though admittedly, I have not dug extensively to know this for certain). Thus, ideally, riteway should be satisfied by whatever version of react the project being tested uses (rather than have its own explicit version requirement). Options 1 and 2 above allow for that, I believe.

If you agree, I would be ecstatic to discuss it further and submit a new PR.

ericelliott commented 2 years ago

Closing in favor of auto-updates