-
Notifications
You must be signed in to change notification settings - Fork 70
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: add compatibility checks w/ semver
#424
Merged
ezolenko
merged 5 commits into
ezolenko:master
from
agilgur5:fix-rollup-load-backward-compat
Sep 20, 2022
Merged
fix: add compatibility checks w/ semver
#424
ezolenko
merged 5 commits into
ezolenko:master
from
agilgur5:fix-rollup-load-backward-compat
Sep 20, 2022
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
…versions - this is my bad, I didn't realize `this.load` came out _much_ later than `this.resolve` - c.f. https://github.com/rollup/rollup/blob/master/CHANGELOG.md#2600 - since we're backward-compatible w/ Rollup `1.26.3`, add in a version check for the type-only fix - the type-only fix will error out in Rollup versions less than `2.60.0`, so instead of erroring out, just skip this functionality - TODO: should move back to using `semver` that was removed in 08d2f5b
- reverts the `semver` removal in 08d2f5b - add back the TS version range check - add a Rollup version range check, fixing an old issue - check `this.load` Rollup version with `semver` instead of string comparison - add `semver` to `external`s list - also remove `resolve` as it's no longer used as of 74f6761 - and re-organize the list so that Node built-ins are in one section while deps are in another - makes it clearer what we're marking as external
- so it doesn't just silently skip the type-only fix on Rollup versions <2.60.0
agilgur5
added
kind: bug
Something isn't working properly
kind: feature
New feature or request
scope: dependencies
Issues or PRs about updating a dependency
kind: dx
Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc
labels
Sep 18, 2022
Ack, integration tests fail because the |
- `TS_VERSION_RANGE`, `ROLLUP_VERSION_RANGE`, `RPT2_VERSION` were previously only defined during builds, so would cause tests to fail - add these vars as (namespaced) globals in the Jest config so that they can be used in tests too - if they don't exist on `global` (or if `global` doesn't exist), then use the built strings instead - see in-line comments for more details - also reorder `re` placement to match the usage order in the code - and fix lint issues (`no-string-literal`)
agilgur5
force-pushed
the
fix-rollup-load-backward-compat
branch
from
September 18, 2022 01:46
4b2e74f
to
2e39986
Compare
agilgur5
added
the
scope: tests
Tests could be improved. Or changes that only affect tests
label
Sep 18, 2022
Fixed the tests and lint issues now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
kind: bug
Something isn't working properly
kind: dx
Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc
kind: feature
New feature or request
scope: dependencies
Issues or PRs about updating a dependency
scope: tests
Tests could be improved. Or changes that only affect tests
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
Add several compatibility checks using
semver
Details
A few commits here:
hotfix:
this.load
was added in Rollup2.60.0
; don't run on older versionsthis.load
came out much later thanthis.resolve
1.26.3
, add in a version check for the type-only fix2.60.0
, so instead of erroring out, just skip this functionalitysemver
that was removed in 08d2f5bdeps: add back
semver
and use for version checkingsemver
removal in 08d2f5b$ROLLUP_VERSION_RANGE
, fixes Detect incompatible Rollup versions #9this.load
Rollup version withsemver
instead of string comparisonsemver
toexternal
s listresolve
as it's no longer used as of docs/clean: formally deprecaterollupCommonJSResolveHack
#367dx: add a nice warning when
this.load
is not supportedfix(test): add globals for strings that are replaced during build
TS_VERSION_RANGE
,ROLLUP_VERSION_RANGE
,RPT2_VERSION
were previously only defined during builds, so would cause tests to failglobal
(or ifglobal
doesn't exist), then use the built strings insteadre
placement to match the usage order in the codeno-string-literal
)Review Notes
Put a few commits together b/c these are all very related (would be dependent on each other) and only make a few changes to the source code anyway.
I also built the
dist/
files to make sure that addingsemver
and$ROLLUP_VERSION_RANGE
works. NOTE that this may need to be rebuilt prior to release if other PRs are merged inOddly enough, adding
semver
as anexternal
removes it from the builds. When not anexternal
though, it gets added to the builds (additions in thegit diff
).... Not sure what's going on there exactly... Maybe it's only partially rolled up currently?package-lock.json
changes shows it was in the dependency tree already, but I only saw it in devDeps 🤔 Not sure why it was in the build.Making it
external
makes it more logical at least (and allows users to override the dep etc per #80 (comment))