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

Watch copied files #27

Closed
wants to merge 1 commit into from
Closed

Conversation

blake-mealey
Copy link

Re: #5 (Support rollup's watch mode)

This adds all files that are copied directly to Rollup's watch list.

@@ -40,7 +40,7 @@ export default function copy(options = {}) {

return {
name: 'copy',
[hook]: async () => {
async [hook]() {
Copy link
Author

Choose a reason for hiding this comment

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

This was necessary because the this keyword was not available with the unbound arrow function syntax.

See this issue for more details: rollup/rollup#1518 (comment)

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #27 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #27   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          36     38    +2     
  Branches       12     12           
=====================================
+ Hits           36     38    +2
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57df513...d6f1109. Read the comment docs.

@@ -88,6 +88,10 @@ export default function copy(options = {}) {
}

for (const { src, dest } of copyTargets) {
try {
this.addWatchFile(src)
} catch { /* Cannot call addWatchFile after the build has finished. */ }
Copy link
Author

Choose a reason for hiding this comment

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

The addWatchFile method cannot be used after the build has finished, so is not available in all hooks. Rather than hard-coding a list of valid hooks that might change, I figured I'd just try/catch it. When it throws an error, it has the error message seen in the comment

@@ -228,6 +228,93 @@ describe('Copy', () => {
expect(await fs.pathExists('dist/scss-multiple/nested-renamed')).toBe(true)
expect(await fs.pathExists('dist/scss-multiple/nested-renamed/scss-3.scss')).toBe(true)
})

test('Watches copied files in pre-build hook', async () => {
Copy link
Author

Choose a reason for hiding this comment

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

I based these off of the copyOnce test

@vladshcherbin
Copy link
Owner

@blake-mealey hey, thanks for the PR. The question I have - when you add files using addWatchFile, will a change in the added file trigger a rebuild?

@blake-mealey
Copy link
Author

From the documentation: https://rollupjs.org/guide/en/#plugin-context

Adds additional files to be monitored in watch mode so that changes to these files will trigger rebuilds

@vladshcherbin
Copy link
Owner

@blake-mealey the reason why I asked is because I've also found addWatchFile in the docs, but full rebuild is a stopper for me to use it.

I've considered another solution using watcher, something like cheap-watch, but didn't look/try it or alternatives.

@blake-mealey
Copy link
Author

Ahh, I see. So you just want to re-copy the individual file? Makes sense

@vladshcherbin
Copy link
Owner

@blake-mealey yes, I see no reason in triggering a rebuild when non-dependent file is changed/removed.

I think that currently there is no built-in way to add files to watcher with a function to trigger when they are changed/deleted w/o rebuild so another separate watcher should be probably used or smth.

It might be a good idea to search for or open an issue in rollup to get an advice or suggestion about such watcher usage, they are usually kind and responsive :)

@blake-mealey
Copy link
Author

@vladshcherbin yeah that makes total sense and I'm happy to close this. I would still really like this feature so I'll look into using a watcher library. I took a quick look and didn't find any issues on the topic.

It's easy to check if rollup's watch mode is enabled (process.env.ROLLUP_WATCH === true). My only concern would be knowing when to cleanup the watcher. There's no rollup hook that would be suitable I don't think, but maybe something simple like process.on('exit', () => {}) (https://nodejs.org/api/process.html#process_event_exit)?

Just from looking briefly at the one you linked (cheap-watch) vs what seems to be a very popular library (chokidar), here are my thoughts:

  • chokidar has a large community and is even (optionally) used by rollup, so it seems like a good option
  • chokidar would make it easy to implement, because we can just pass it an array of paths or globs and it will just watch them for us
  • cheap-watch isn't as big of a project, so questionable option
  • cheap-watch would be a bit more awkward to implement (and less performant?) because we would tell it to watch the whole project directory, then manually check the paths when changes occur to see if they match our files (so we would be doing work when any file changes)

Given all that, I'd like to try a chokidar approach, being enabled based on process.env.ROLLUP_WATCH, and cleaning itself up on process.on('exit', ...). What do you think?

@vladshcherbin
Copy link
Owner

@blake-mealey yep, process exit can be an option to clean watcher, need to test in some conditions, for example when build fails, throws an error. I believe it should be fine, errors in plugins also use it, same as watcher.

The reason I mentioned cheap-watch is because I've read a conversation in rollup issue - rollup/rollup#2988 (comment), not far ago, and saw it there. Didn't compare them myself though, so don't know what are pros, cons or why alternatives were created :)

@vladshcherbin
Copy link
Owner

@blake-mealey so all in all, I believe your idea is nice and should work fine, I planned to do the same thing one day 👍

@blake-mealey
Copy link
Author

blake-mealey commented Oct 28, 2019 via email

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.

None yet

2 participants