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.

Sign up for GitHub

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

Jump to bottom

Add just the necessary files to rollup watch mode #2007

Merged
merged 2 commits into from
Jun 27, 2024
Merged

Add just the necessary files to rollup watch mode #2007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
760a202
Add just the necessary files to rollup watch mode
vstefanovic97 Jun 26, 2024
c9a9be3
Add rollup as a peerDep to @embroider/addon-dev
vstefanovic97 Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/addon-dev/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@
"scripts": {
"test": "jest"
},
"peerDependencies": {
"rollup": "^4.6.0"
},
"peerDependenciesMeta": {
"rollup": {
"optional": true
mansona marked this conversation as resolved.
Show resolved Hide resolved
}
},
"dependencies": {
"@embroider/core": "workspace:^",
"@rollup/pluginutils": "^4.1.1",
Expand Down
8 changes: 2 additions & 6 deletions packages/addon-dev/src/rollup-keep-assets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can see this issue got resolved

Copy link
Member

Choose a reason for hiding this comment

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

They have a nifty bot that tells you what version of rollup includes the PR 🎉 apparently this was fixed in rollup@4.6.0.

I wonder if we have any peer dependency reference or anything that would allow us to say which versions of rollup are supported 🤔

Copy link
Member

Choose a reason for hiding this comment

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

also if we are changing the "rollup support matrix" this one change could technically make this a breaking change... maybe 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mansona I added the change, take a look when you can

// from within `generateBundle`
buildStart() {
this.addWatchFile(from);
},

// imports of assets should be left alone in the source code. This can cover
// the case of .css as defined in the embroider v2 addon spec.
async resolveId(source, importer, options) {
Expand All @@ -44,6 +38,8 @@ export default function keepAssets({
globs: include,
directories: false,
})) {
this.addWatchFile(join(from, name));

this.emitFile({
type: 'asset',
fileName: name,
Expand Down
4 changes: 2 additions & 2 deletions packages/addon-dev/src/rollup-public-entrypoints.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it will take into account exclude option and exclude files from watch mode as well

});

for (let name of matches) {
this.addWatchFile(path.join(args.srcDir, name));
void-mAlex marked this conversation as resolved.
Show resolved Hide resolved

// the matched file, but with the extension swapped with .js
let normalizedName = normalizeFileExt(name);

Expand Down
Loading