🧶 Add tslib to dependencies#4314
Conversation
| integrity sha512-336iVw3rtn2BUK7ORdIAHTyxHGRIHVReokCR3XjbckJMK7ms8FysBfhLR8IXnAgy7T0PTPNBWKiH514FOW/WSg== | ||
|
|
||
| tslib@^2.1.0, tslib@^2.4.0, tslib@^2.8.1: | ||
| tslib@2.8.1, tslib@^1.8.1, tslib@^1.9.3, tslib@^2.0.0, tslib@^2.0.1, tslib@^2.1.0, tslib@^2.3.0, tslib@^2.4.0, tslib@^2.8.1: |
There was a problem hiding this comment.
I'm not convinced about v1.8.1 and v1.9.3 being coerced to v2. Maybe this manual edit would be enough?
| tslib@2.8.1, tslib@^1.8.1, tslib@^1.9.3, tslib@^2.0.0, tslib@^2.0.1, tslib@^2.1.0, tslib@^2.3.0, tslib@^2.4.0, tslib@^2.8.1: | |
| tslib@^2.1.0, tslib@^2.4.0, tslib@^2.8.1, tslib@^2.0.0, tslib@^2.0.1, tslib@^2.3.0: |
And:
- remove the resolution
- remove the bloc for 2.5.0
- keep the block for the v1 versions.
Not sure if a future yarn install would split the block into two again, one for 2.8.1 and another for 2.5.0. WDYT?
There was a problem hiding this comment.
Why would this be an advantage? I'd expect the lock file to be mostly managed by yarn, not edited manually to avoid issues like this in the future. But it's good to know also the advantages if any of your proposal.
There was a problem hiding this comment.
Because if we add the resolution, that will force all packages that depend on tslib@^1.8.1 and tslib@^1.9.3 to use 2.8.1, which is a different major. That could cause issues somewhere else.
There was a problem hiding this comment.
I don't think manually editing then lock file is good idea.
According to ✨ tslib is backwards compatible and this should be safe (take it with a pinch of salt).
I can investigate further if we can avoid both options, I also didn't like forcing to use v2.
There was a problem hiding this comment.
Bob actually suggested an alternative approach - to put tslib as a direct dep. But so far fails to suggest and meaningful advantage to it. So I will approve but I will leave between you to figure out which of all approaches is best.
There was a problem hiding this comment.
After a long talk with Claude I think this is the safest solution @akostadinov . The only issue I have is tslib could be misidentified as a rogue dependency and be removed. I added a justification in the commit message to prevent this.
There was a problem hiding this comment.
After a long chat it sounded morelike your original solution was better but As I said, you decide.
@patternfly/react-icons uses tslib at runtime but only declares it as a devDependency. It worked by accident via yarn hoisting from sibling packages until the webpack-dev-server bump in #4303 disrupted hoisting.
josemigallas
force-pushed
the
fix_webpack-dev-server_bump
branch
from
7351f2e to
387cef0
Compare
| "showdown": "2.1.0", | ||
| "swagger-client": "^3.36.1", | ||
| "swagger-ui": "5.27.1", | ||
| "tslib": "2.8.1", |
There was a problem hiding this comment.
Maybe ^2.0.0? That's the exact dependency used by patternfly/react
Line 2353 in 387cef0
I assume that's the one ract-icons should declare as well, right?
It's resolved as 2.5.0, not 2.8.1, but I guess it would work.
Anyway approving.
There was a problem hiding this comment.
It works (on my machine) and it should be compatible according to semver. Since it's our dependency now I think it's in our best interest to use the latest version compatible.
Also, dependably will go crazy if we install an older version haha
3 participants
#4303 broke the image because shared dependencies with patternfly.
✨ Why this happened: Even though webpack-dev-server seems unrelated to PatternFly, it shares the same dependency tree. When
webpack-dev-serverupdated, it pulled in new versions of dependencies (likememfs) that required newer tslib. Yarn's hoisting algorithm created a split where different packages got different tslib versions, breaking webpack's module resolution.UPDATE:
Adding a resolution will force all packages to used this version, packages that may depend on tslib@1. It's safer to simply add tslib as a dependency to ensure it is available for react-icons.
The reason exactly why tslib is not available after webpack-dev-server bump is unknown: Claude blames yarn's hoisting algorithm and its dependency resolver. 😕