Skip to content
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
merged 5 commits into from Sep 20, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Sep 18, 2022

Summary

Add several compatibility checks using semver

Details

A few commits here:

  • hotfix: this.load was added in Rollup 2.60.0; don't run on older versions

    • this is my bad, I didn't realize this.load came out much later than this.resolve
    • 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
  • deps: add back semver and use for version checking

  • dx: add a nice warning when this.load is not supported

    • so it doesn't just silently skip the type-only fix on Rollup versions <2.60.0
  • fix(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 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)

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 adding semver and $ROLLUP_VERSION_RANGE works. NOTE that this may need to be rebuilt prior to release if other PRs are merged in

Oddly enough, adding semver as an external removes it from the builds. When not an external though, it gets added to the builds (additions in the git 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))

…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 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
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Sep 18, 2022

Ack, integration tests fail because the $TS_VERSION_RANGE and $ROLLUP_VERSION_RANGE are not substituted yet in the tests (they're only replaced in the built files)... will need to handle that somehow...

- `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 agilgur5 force-pushed the fix-rollup-load-backward-compat branch from 4b2e74f to 2e39986 Compare September 18, 2022 01:46
@agilgur5 agilgur5 added the scope: tests Tests could be improved. Or changes that only affect tests label Sep 18, 2022
@agilgur5
Copy link
Collaborator Author

Fixed the tests and lint issues now

@ezolenko ezolenko merged commit 3ef3289 into ezolenko:master Sep 20, 2022
@agilgur5 agilgur5 deleted the fix-rollup-load-backward-compat branch July 2, 2023 21:23
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect incompatible Rollup versions
2 participants