deps: upgrade tslib to ^2.4.0, remove @yarn-tool/resolve-package
#326
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- tslib 2.4.0 is forward and backward-compatible with older and newer
Node exports mechanisms, so the Node 17 error should no longer be
present
- it has the older `./` and the newer `./*` in its package exports,
which should allow for `package.json` to be read in both older and
newer implementations
- this allows us to remove the extra dep on `@yarn-tool/resolve-package`
as well
- other than less unnecessary deps being good,
`@yarn-tool/resolve-package` is also a not well-documented package
with very few users, which does not make for a good security posture
for rpt2 (which has historically prioritized supply chain security
in other issues around deps) or, in particular, its consumers, which
there are very many of (in contrast with `@yarn-tool`)
- per my issue comment, we could also have avoided the extra dep prior
to the tslib upgrade by resolving to absolute paths, as Node only
does a "weak" encapsulation of relative imports
- test: add a small unit test for tslib.ts to ensure that this method
works and passes on different Node versions in CI
- more a smoke test that it runs at all, the testing is additional
and a bit duplicative of the source tbh
agilgur5
added
scope: dependencies
agilgur5
force-pushed
the
deps-upgrade-tslib-remove-yarn-tool
branch
from
d52e27c to
189aaa5
Compare
|
Tests pass on all Node versions, and I temporarily added a commit with passing tests to check Node 17 specifically as well. |
This was referenced May 16, 2022
agilgur5
added
the
kind: internal
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #286 in a simpler way by upgrading
tslibinstead of introducing any new depsImplements the approach I proposed in #286 (comment)
Details
tslib2.4.0is forward and backward-compatible with older and newer Node exports mechanisms, so the Node 17 error should no longer be presentpackage.jsonafter Update package.json: changed pattern "./" to "./*" microsoft/tslib#135./and the newer./*in its package exports, which should allow forpackage.jsonto be read in both older and newer implementationsthis allows us to remove the extra dep on
@yarn-tool/resolve-packageas well@yarn-tool/resolve-packageis also a not well-documented package with very few users, which does not make for a good security posture for rpt2 (which has historically prioritized supply chain security in other issues around deps (c.f. Do not bundle dependencies #80, chore: should not lock deps version #293 (comment)) or, in particular, its consumers, which there are very many of (in contrast with@yarn-tool)tslibhelper library #286 (comment), we could also have avoided the extra dep prior to thetslibupgrade by resolving to absolute paths, as Node only does a "weak" encapsulation of relative importstest: add a small unit test for
tslib.tsto ensure that this method works and passes on different Node versions in CI