nollup icon indicating copy to clipboard operation
nollup copied to clipboard

watch include options does not seem to work with path above

Open wighawag opened this issue 4 years ago • 18 comments

I have the following watch option:

 watch: {
    clearScreen: false,
    include: ['src', path.resolve(__dirname, '../test/dist')],
  },

and while it works when I change stuff in src, it does not when I change stuff in ../test/dist I also tried without resolve and same issue.

Is that because nollup will not look in parent folders ?

wighawag avatar Jul 06 '20 20:07 wighawag

Hey @wighawag!

I'm a bit confused, can you describe your folder structure? Not sure I understand where the rollup.config.js file is and the test directory.

PepsRyuu avatar Jul 06 '20 20:07 PepsRyuu

I am using yarn workspace and the test folder is a library in the workspace. I actually do that so I can develop the library along side the project, but nollup do not recompile on changes happening there

wighawag avatar Jul 06 '20 21:07 wighawag

so to be clear the folder structure is as followed :

root/webapp
root/webapp/rollup.config.js
root/webapp/src
root/test
root/test/dist
root/test/src

wighawag avatar Jul 06 '20 21:07 wighawag

That would definitely cause issues. Nollup assumes that at all directories for watching (regardless of include or exclude) are under the current working directory provided by process.cwd(). It passes this as an argument to chokidar, and then applies the filtering rules based on that.

I'm not entirely sure what the appropriate solution for this. Having a file watcher watch files outside the current working directory can be very expensive in terms of performance. The more files you're watching, the slower file watching becomes, so it's generally ill-advised to expand the scope of watching. Will have to see if it's possible to add additional directories to chokidar based on the include parameter.

PepsRyuu avatar Jul 06 '20 22:07 PepsRyuu

there's a "watch" option for chokidar, where we could send watch paths via .nolluprc.js, then we could watch specific paths for custom projects.

In dev-server

 app.use(nollupDevMiddleware(app, config, {
        hot: options.hot,
        verbose: options.verbose,
        hmrHost: options.hmrHost,
        contentBase: options.contentBase,
        publicPath: options.publicPath,
        watch: options.watch,    // adding this is only update need for this to work, rest is there already
    }));

Hope you could add this option... basically i couldn't use nollup because of this...

Dulanjala007 avatar Jul 09 '20 15:07 Dulanjala007

well, include doesn't work the way I thought: https://rollupjs.org/guide/en/#watchinclude

Limit the file-watching to certain files. Note that this only filters the module graph but does not allow to add additional watch files

piotr-cz avatar Jul 09 '20 15:07 piotr-cz

problem for me was being unable to provide watch paths via options, in nollupDevMiddleware it already checks for a watch param ( chokidar.watch(options.watch || process.cwd()), but watch param wasn't passed in from dev-server, since it was missing the watch: options.watch, where it pass options to middleware (in line :- app.use(nollupDevMiddleware(app, config, { //options }) seems a small issue of missed property...

Dulanjala007 avatar Jul 09 '20 16:07 Dulanjala007

@Dulanjala007 I think the options.watch is just and undocumented thing, as it falls back to cwd.

piotr-cz avatar Jul 09 '20 16:07 piotr-cz

The watch param there is legacy and shouldn't be trusted at all (hence not documented, also likely broken due to the below). The official approach at the moment is with the rollup.config.js to stay consistent with Rollup so that it's interchangeable. The idea of having watch configuration split over two files isn't very appealing and likely would lead to confusion. There would have to be a unified API that can work.

Also worth noting, because @rollup/pluginutils is being used for the include/exclude options, those are bound to process.cwd(). https://github.com/rollup/plugins/tree/master/packages/pluginutils#createfilter

PepsRyuu avatar Jul 09 '20 16:07 PepsRyuu

Relevant issue: https://github.com/rollup/plugins/issues/490

PepsRyuu avatar Jul 09 '20 16:07 PepsRyuu

@wighawag This https://github.com/rollup/rollup/issues/3414#issuecomment-626095184 may be related to original issue

piotr-cz avatar Jul 09 '20 16:07 piotr-cz

@piotr-cz so you mean I should use extra like watch: { extra: ... } ?

wighawag avatar Jul 10 '20 07:07 wighawag

@wighawag I've just dropped the link here, that I thought is related - I'm not sure if it will help.

piotr-cz avatar Jul 13 '20 12:07 piotr-cz

An option for a watch outside src dir would be amazing! I need to watch node_modules/my-component during development using npm link.

Is there a way today, @PepsRyuu?

frederikhors avatar Mar 25 '21 15:03 frederikhors

+1-ing this, am excited to try switching our dev tooling to nollup, have the same issues. Currently using rollup and our directory structure is roughly like this:

/app-1
  /various-code
  package.json    - react, other dependencies
/app-2
[...]
/cli
  /tooling
    /build
      package.json    - rollup, nollup, etc.
      rollup.config.js

rollup doesn't require compiled files to be inside the same directory as rollup is executed from, so this has been working well. It also means we can cd into one tooling directory and there's an environment (package.json) there that can compile all parts of our app, which is a really nice way to do things.

(In fact, rollup (tested on 3.10.0) is smart enough to watch everything under the entry file, so there was zero ext7ra config to get this working. i.e. with --watch enabled, rollup seems to understand that the entry is ../../app-1/app-1.js and so it automatically watches everything under ../../app-1/.)

Above it's suggested that being able to watch files outside the cwd would represent a performance penalty — not sure I agree, i.e. I can't see that watching ./src/**/*.js would give a performance advantage vs watching '../../src/**/*.js — am I missing something obvious there?

PS editing to add thanks for this awesome tool, was very excited to find it :D

bhallstein avatar Jan 23 '23 11:01 bhallstein

Thanks for the info @bhallstein!

This is probably worth revisiting as I'm sure quite a bit has changed in the meantime since this issue was first raised. Been a bit pre-occupied with life in general lately, but hopefully I can have a look at it soon. :)

PepsRyuu avatar Jan 23 '23 13:01 PepsRyuu

Some research notes:

So having a look into this, I replicated the type of folder structure you have above, and I can observe that Rollup does indeed watch imported files outside of the current working directory, while Nollup will ignore the files (but still include them in this.getWatchFiles for plugins).

Rollup achieves this because its rollup.watch functionality has internal access to the watchFiles implementation via rollup.rollupInternal. The concept of watching is baked into the Rollup architecture. Nollup does not implement .watch, instead, the watching feature is completely decoupled and put into the dev middleware. The problem is, I'm not entirely sure how to replicate this feature without coupling the dev middleware into the private implementation of Nollup, architecturally it's just not clean and means that users who have their own dev middleware won't be able to replicate the same functionality without relying on private implementation details either.

On the other hand, it might mean I have to look into implementing nollup.watch and refactor the dev middleware to use this function instead. But as far as I can tell, implementing the .watch API will not be compatible with an in-memory architecture required by the dev middleware, as Rollup's .watch always writes to disk, whereas the dev middleware relies on .generate() solely. The watch API also doesn't give you the generated bundles either.

Will need to think about this more.

PepsRyuu avatar Jan 26 '23 22:01 PepsRyuu

@bhallstein Does this branch support your use case? https://github.com/PepsRyuu/nollup/pull/242

PepsRyuu avatar Jan 26 '23 22:01 PepsRyuu