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 include options does not seem to work with path above #102

Open
wighawag opened this issue Jul 6, 2020 · 18 comments
Open

watch include options does not seem to work with path above #102

wighawag opened this issue Jul 6, 2020 · 18 comments

Comments

@wighawag
Copy link

wighawag commented Jul 6, 2020

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 ?

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jul 6, 2020

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.

@wighawag
Copy link
Author

wighawag commented Jul 6, 2020

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
Copy link
Author

wighawag commented Jul 6, 2020

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

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jul 6, 2020

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.

@Dulanjala007
Copy link

Dulanjala007 commented Jul 9, 2020

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...

@piotr-cz
Copy link
Contributor

piotr-cz commented Jul 9, 2020

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

@Dulanjala007
Copy link

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...

@piotr-cz
Copy link
Contributor

piotr-cz commented Jul 9, 2020

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

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jul 9, 2020

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
Copy link
Owner

PepsRyuu commented Jul 9, 2020

Relevant issue: rollup/plugins#490

@piotr-cz
Copy link
Contributor

piotr-cz commented Jul 9, 2020

@wighawag
This rollup/rollup#3414 (comment) may be related to original issue

@wighawag
Copy link
Author

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

@piotr-cz
Copy link
Contributor

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

@frederikhors
Copy link

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?

@bhallstein
Copy link

bhallstein commented Jan 23, 2023

+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

@PepsRyuu
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

@bhallstein Does this branch support your use case? #242

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

No branches or pull requests

6 participants