Add just the necessary files to rollup watch mode #2007
Conversation
| @@ -14,12 +14,6 @@ export default function keepAssets({ | |||
| return { | |||
| name: 'copy-assets', | |||
|
|
|||
| // Prior to https://github.com/rollup/rollup/pull/5270, we cannot call this | |||
There was a problem hiding this comment.
From what I can see this issue got resolved
There was a problem hiding this comment.
They have a nifty bot that tells you what version of rollup includes the PR 🎉 apparently this was fixed in [email protected].
I wonder if we have any peer dependency reference or anything that would allow us to say which versions of rollup are supported 🤔
There was a problem hiding this comment.
also if we are changing the "rollup support matrix" this one change could technically make this a breaking change... maybe 🤔
There was a problem hiding this comment.
@mansona that is a good point, I can add rollup as a peerDep to @embroider/addon-dev's package.json file if you feel like that is the right approach?
And it might we a major release, if we take that into account?
There was a problem hiding this comment.
@mansona looks like all the rollup maintained plugins include rollup as a peerDep
Like for example here in @rollup/babel, so I guess this would be the way to go
https://github.com/rollup/plugins/blob/master/packages/babel/package.json#L58
There was a problem hiding this comment.
so the fact that rollup does it really makes me feel like we should do the same thing. And we need to declare the minimum version of rollup that we support
if you want to go ahead and add that in this PR we can mark it as breaking and it will automatically get picked up and released correctly 👍
There was a problem hiding this comment.
@mansona I added the change, take a look when you can
| @@ -16,14 +16,14 @@ export default function publicEntrypoints(args: { | |||
| return { | |||
| name: 'addon-modules', | |||
| async buildStart() { | |||
| this.addWatchFile(args.srcDir); | |||
|
|
|||
| let matches = walkSync(args.srcDir, { | |||
| globs: [...args.include, '**/*.hbs', '**/*.ts', '**/*.gts', '**/*.gjs'], | |||
| ignore: args.exclude, | |||
There was a problem hiding this comment.
Now it will take into account exclude option and exclude files from watch mode as well
|
@simonihmig yea I'm happy this is covered 👍 LGTM |
|
Thank you @mansona, @void-mAlex, @NullVoxPopuli , @simonihmig for the reviews and merging this so quickly! |
|
Thanks for your contribution! 👍🎉 |
|
I do not think this was correct. Now it does not detect new files added to the src folder |
|
I think the right solution would have been to either setup watch exlcude or watch include |
I noticed when working on a v2 addon that it was considering all files inside
srcfor watch mode.This is not ideal, as in some cases we want to have exclusions, eg. if we have something (eg postcss-d-ts) that generated type declaration files for our css we don't want to have those files included in watch mode or else they will trigger infinite re-builds.
Made the change so we explicitly add only the files we need