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(typescript): utilize 'this.meta.watchMode' #449

Merged
merged 3 commits into from
Jul 6, 2020

Conversation

dionysiusmarquis
Copy link
Contributor

@dionysiusmarquis dionysiusmarquis commented Jun 10, 2020

Rollup Plugin Name: typescript

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

BREAKING CHANGES: Changed rollup peer dependency to rollup@^2.14.0

List any relevant issue numbers:
rollup/rollup#3608
#418
Fix: #478

Description

I applied prettier and added the new this.meta.watchMode (see rollup/rollup#3616). Checking process.env.ROLLUP_WATCH will only work in rollup -w context. Checking for this.meta.watchMode will also work in rollup.watch context. In the future plugins should preferable utilize this.meta.watchMode.

@dionysiusmarquis dionysiusmarquis changed the title Issue/418 fix(typescript): utilize 'this.meta.watchMode' Jun 10, 2020
@shellscape shellscape requested a review from NotWoods June 10, 2020 22:16
@shellscape
Copy link
Collaborator

Thanks for opening a PR. Unfortunately it looks like you have a failure on our analysis CI step and both Node 10 and 12 are failing tests. We also have a strict policy on not accepting fixes without accompanying tests. To test this one you might have to simulate watching a rollup build.

@dionysiusmarquis
Copy link
Contributor Author

Maybe the tests are failing because the rollup version is not ^2.14.0 🤔
watchMode will not exist in the PluginContextMeta Interface in older rollup versions.

[!] (plugin typescript) Error: @rollup/plugin-typescript TS2339: Property 'watchMode' does not exist on type 'PluginContextMeta'.
 
src/index.ts (65:60)
 65       if (process.env.ROLLUP_WATCH !== 'true' && this.meta.watchMode !== true) {
                                                              ~~~~~~~~~
 Error: @rollup/plugin-typescript TS2339: Property 'watchMode' does not exist on type 'PluginContextMeta'. 
    at error (/home/circleci/project/node_modules/.pnpm/registry.npmjs.org/rollup/2.2.0/node_modules/rollup/dist/shared/rollup.js:10120:30)

@dionysiusmarquis
Copy link
Contributor Author

@shellscape How can I ensure the tests are running against rollup@^2.14.0? I did not add any new functionality. I just changed one variable to test against before killing the Typescript watch program.

@shellscape
Copy link
Collaborator

@dionysiusmarquis the same as you would any other package - you need to ensure that the right version is installed in devDeps. Because this will rely upon a particular patch version, the PR contains breaking changes, and that means it must be bumped to a new major version. The breaking change is that anyone using prior versions must now have a peerDep of rollup 2.14.0+

@dionysiusmarquis
Copy link
Contributor Author

@shellscape I don't know if I understand you correctly.

Actually this won‘t change or break anything if you use an older rollup version. It will behave exactly like it would before this PR. This PR will reenable the ability to use the watch Hook in rollup.watch context. You need rollup@^2.14.0 if you want to utilize the Typescript Interface of PluginContextMeta. Which, as far as I can tell only necessary in the test environment.
If you use this plugin with an older rollup version from node_modules this.meta.watchMode will be undefined and everything will behave just like before this PR.

If you mean we have to ensure that the watch hook will always be utilized in any rollup watch context coherently. Then yes, we should change the peerDep.

@dionysiusmarquis dionysiusmarquis force-pushed the issue/418 branch 2 times, most recently from e13a87c to f76a94f Compare June 14, 2020 17:06
Copy link
Member

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're increasing the peer dependency, this needs to be marked as a breaking change.

@shellscape shellscape merged commit d3aae89 into rollup:master Jul 6, 2020
LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
* style(typescript): prettier index.js and watchProgram.js

* fix(typescript): utilize 'this.meta.watchMode'

* fix(typescript): set `rollup` peer dependency to version 2.14.0 or higher
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Watching broken in TypeScript 4.1.2 and newer (including latest version 5.x+)
3 participants