Push manifest does not adhere custom changes in preact config file

[oliverfindl]

What is the current behaviour?

When I change output paths via preact.config.js file, generated push-manifest.json file is wrong.

Steps to reproduce:

1. Change output paths via preact.config.js file:

export default (config, env, helpers) => {

	config.output.filename = "scripts/" + config.output.filename;
	config.output.chunkFilename = "scripts/" + config.output.chunkFilename;

	const { plugin: babelEsmPlugin } = helpers.getPluginsByName(config, "BabelEsmPlugin")[0];
	babelEsmPlugin.options_.filename = "scripts/" + babelEsmPlugin.options_.filename;
	babelEsmPlugin.options_.chunkFilename = "scripts/" + babelEsmPlugin.options_.chunkFilename;

	const { plugin: miniCssExtractPlugin } = helpers.getPluginsByName(config, "MiniCssExtractPlugin")[0];
	miniCssExtractPlugin.options.filename = "styles/" + miniCssExtractPlugin.options.filename;
	miniCssExtractPlugin.options.chunkFilename = "styles/" + miniCssExtractPlugin.options.chunkFilename;

};

2. Build:

$ npm run build

3. Check generated push-manifest.json file:

$ cat build/push-manifest.json

{"/":{"undefined":{"type":"script","weight":1}}}

What is the expected behaviour?

Generate correct push-manifest.json file.

Please mention any other relevant information:

preact-cli/packages/cli/lib/lib/webpack/create-load-manifest.js

Line 9 in aea6fed

for (let filename in assets) {

In filename variable are not basenames of assets, but whole path, therefore regular expressions won't match.

scripts/bundle.945f9.js
scripts/bundle.d7579.esm.js
scripts/polyfills.60e68.js
scripts/polyfills.aae10.esm.js
styles/bundle.fa67f.css
sw-esm.js
sw.js

preact-cli/packages/cli/lib/lib/webpack/create-load-manifest.js

Line 16 in aea6fed

} else if (/^bundle(.+)\.css$/.test(filename)) {

preact-cli/packages/cli/lib/lib/webpack/create-load-manifest.js

Line 18 in aea6fed

} else if (/^bundle(.+)\.js$/.test(filename)) {

Please paste the results of npx preact-cli info here.

Environment Info:
  System:
    OS: Linux 4.15 Ubuntu 16.04.7 LTS (Xenial Xerus)
    CPU: (4) x64 Intel(R) Core(TM) i5-5200U CPU @ 2.20GHz
  Binaries:
    Node: 17.6.0 - /usr/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 8.5.1 - /usr/bin/npm
  Browsers:
    Chrome: 99.0.4844.51
    Firefox: 88.0
  npmPackages:
    preact: ^10.3.1 => 10.6.6
    preact-cli: ^3.0.0 => 3.3.5
    preact-render-to-string: ^5.1.20 => 5.1.20

[oliverfindl]

This issue also affects --preload flag.

[rschristian]

I'd lean towards calling this behavior correct, honestly.

If you begin to alter expected file names through the config, there's no way for us to be aware of this, so you more or less become responsible for that change -- this means fixing locations in which our expectations will no longer be true.

Altering behavior here would also likely be a breaking change.

[oliverfindl]

Hello @rschristian,

thanks for your fast reply!

I'd lean towards calling this behavior correct, honestly.

Well, I would not say so. In this case, it should not generate push-manifest.json file instead of using that "undefined" value in it as in current state. --preload flag seems to ignore this behavior, which IMO is correct.

If you begin to alter expected file names through the config, there's no way for us to be aware of this, so you more or less become responsible for that change -- this means fixing locations in which our expectations will no longer be true.

I'm aware that this issue is "tax" for opinionated approach. So let me ask, what is the preferred way in preact-cli to achieve project structure, where different assets will be placed in different directories instead of all in project root? E. g.:

• js in /scripts (as in my snippet above)
• css in /styles (as in my snippet above)
• fonts in /fonts (I'm currently using modified options for file-loader and url-loader via preact.config.js file)
• images in /images (I'm currently using modified options for file-loader and url-loader via preact.config.js file)
• audio & video in /media (I'm currently not using any media in my project, but I might in future)
• etc.

Thanks.

[rschristian]

I'd lean towards calling this behavior correct, honestly.

Well, I would not say so. In this case, it should not generate push-manifest.json file instead of using that "undefined" value in it as in current state. --preload flag seems to ignore this behavior, which IMO is correct.

Sorry, misspoke there. Definitely agree, we should fix that to output nothing (and alter assumptions about a push-manifest.json always being created) in cases like this. However, the fact that the output does look like that (at least in some ephemeral form) is correct.

So let me ask, what is the preferred way in preact-cli to achieve project structure, where different assets will be placed in different directories instead of all in project root? E. g.:

There is no preferred way. If you really need to make those changes, you'll likely need to make a fair number adjustments to the config.

Is there any particular reason you want to make these changes? File servers should be more than capable of serving any file type from any location. You shouldn't need to segregate them out.

[oliverfindl]

Hello @rschristian,

Is there any particular reason you want to make these changes? File servers should be more than capable of serving any file type from any location. You shouldn't need to segregate them out.

In Europe, court already fined some company for not self-hosting fonts (while using Google Fonts CDN instead), because it led to leaking user IP addresses (in EU is IP address alone considered as PII) without user consents out of EU. So now we are required to self-host all fonts - or to be precise, all static files ourselves to be safe. If project uses few fonts, each with few unicode ranges (latin, latin-ext, etc.), few font variants (weights), it will end up in tens - even hundreds of font files in project root. Therefore I want them in separate directory, so I can navigate without problems (actually I don't need to touch fonts ever again, so there is no reason for them to pollute project root). When I was doing this, I distributed all files into my preferred directory structure (like in my previous comment). I wouldn't say, it is technical issue, just I don't like a big mess of files in project root.

Thanks.

[rschristian]

So that context helps, a lot.

If you're willing to forgo /scripts and /styles, you can just copy directories from your source to build. For example, in all of our templates, we have a src/assets directory. This is copied directly into the output (build/assets) so all paths stay the same. Normally this is what you'd load up with fonts, images, media, etc., but it really is just a special-cased directory name. You could add anything to the copy plugin and have it work the same.

So src/fonts, src/images (etc), would become build/fonts/, build/images/.

// preact.config.js

export default (config, env, helpers) => {
    const copyPlugin = helpers.getPluginsByName(config, 'CopyPlugin')[0];
    if (copyPlugin) {
        const { plugin } = copyPlugin;
        // Looks a bit funny, but there's an implicit "<src input>" for "from", and "<build output>" for "to"
        plugin.patterns.push({ from: 'fonts', to: 'fonts' }); // etc.
    }
}

Now, scripts and styles are a lot more involved as they are used for prerendering, file preloading, etc. These end up being a lot more "hard-coded" basically by necessity. Static assets though? Copying them to the output is no problem.

[oliverfindl]

Hello @rschristian,

I'm familiar with webpack-copy-plugin and webpack itself, already using it in my preact.config.js with other changes. Currently I'm using fonts from fontsource package, therefore they are not in my src directory. I already updated webpack / preact-cli config file to copy them into /fonts directory and postcss with purgecss to remove unused fonts - so no problem here. I would strongly prefer to have all files distributed in particular directories instead of root, but I understand why it is not possible. Currently I have only MVP of my project, building it with --no-sw --no-esm --no-prerender flags and all working as expected (except wrong push-manifest.json file being generated and --preload flag not working, which both I can solve manually) - but it will be issue with "full" build. I will try to fiddle with it later and probably fallback back to react (there is another issue, that is kinda blocking me).

Now, scripts and styles are a lot more involved as they are used for prerendering, file preloading, etc. These end up being a lot more "hard-coded" basically by necessity. Static assets though? Copying them to the output is no problem.

Checked source file (lines 16 and 18), responsible for this issue and I want to ask, if just simple const { basename } = require('path'); and basename(filename) in .test() method could not solve this issue? From my limited understanding (as I don't understand underlying logic), it should fix condition with regular expression, while preserving original path of that file. I tried to test it by adding this change directly into my node_modules source file and it solved this issue for me, push-manifest.json is generated with valid content. But I did not test it without flags mentioned above, because there are currently other roadblocks in my project.

Thanks.

[rschristian]

I want to ask, if just simple const { basename } = require('path'); and basename(filename) in .test() method could not solve this issue?

All that would need to change is the ^ in the regex (which means "start"). While sure, that could solve your immediate issue, that's a very different test that's no where near as sound (not that our current setup necessarily is either, as proven by this issue).

Let's say a user has a plugin that outputs additional assets, including a widget/bundle.<hash>.css. This is fine, as it does not match that regex pattern. But under your suggestion, it would. How do we know which is the main CSS file and which is not? If there's multiple bundle.<something>.css files that are output, we have no way of knowing which is which.

[oliverfindl]

Hello @rschristian,

I want to ask, if just simple const { basename } = require('path'); and basename(filename) in .test() method could not solve this issue?

All that would need to change is the ^ in the regex (which means "start"). While sure, that could solve your immediate issue, that's a very different test that's no where near as sound (not that our current setup necessarily is either, as proven by this issue).

Of course, removing caret would solve this with even less effort, but IMO path.basename() seems better / safer solution.

Let's say a user has a plugin that outputs additional assets, including a widget/bundle..css. This is fine, as it does not match that regex pattern. But under your suggestion, it would. How do we know which is the main CSS file and which is not? If there's multiple bundle..css files that are output, we have no way of knowing which is which.

Well as I said before, I don't understand underlying logic enough, but first things on my mind are, not using generic name as bundle, but e. g.: preact-bundle (which might not solve your example issue with widget if it would be also preact-based). Or even make that name an option. Or generate it as random string, store it in file and read it from there later when needed. Maybe access to hashes in filenames of assets (precise asset name with hash in it) can be somehow gained. Or simple webpack loader / plugin can store them in file. Honestly, don't know. Correct solution would be to compare filename - or to be precise - filepath with options from all webpack loaders / plugins, which could be changed, as in my case. Because on one hand, webpack / preact-cli allows to do such things and on other hand, preact-cli ignoring it and using its defaults - which is of course fine for some hello world project, but it hits the wall in projects like mine - but this is my personal issue with every opinionated library / framework (looking at you vite).

Thanks.

[rschristian]

Or even make that name an option.

More CLI flags = bad, especially for something like file names, which really shouldn't need to be altered. More options make it harder for new users to learn and more for us to test and maintain.

Or generate it as random string, store it in file and read it from there later when needed. Or simple webpack loader / plugin can store them in file.

I'm extremely reluctant to add hacky fixes for self-inflicted problems.

Because on one hand, webpack / preact-cli allows to do such things and on other hand, preact-cli ignoring it and using its defaults

If it's not clear, using your preact.config.js is how you run around preact-cli and make changes that differ from what we've set. It's not that different from CRA's eject in that you now "own" some of the config (though unlike CRA, it's just a slice of the config, not the entire thing).

You're taking our config, cutting out parts you don't agree with, but not altering the dependants downstream. You absolutely could replace our manifest generating plugin with your own, which would realistically be the recommendation.

Maybe access to hashes in filenames of assets (precise asset name with hash in it) can be somehow gained.

As far as I know, Webpack does not expose output file/chunkname to plugins, which is why this was built as it was. I'll look into it further, certainly could've been added over the years.

Regardless, as this is just a cosmetic issue, and in your build output no less, I think my advice has to be to just skip editing file names if you don't absolutely need it.

[oliverfindl]

Hello @rschristian,

got it! You can close this issue and mark it as wont-fix after that push-manifest.json is fixed (not created if no assets are detected). I got workaround solution for my use-case, so all good at my end.

Thanks.

[rschristian]

Sorry for the long-time-no-comment.

Looks like webpack-manifest-plugin handles this quite well (to my surprise, quick glance at its docs don't do it justice). Fairly minimal too, so I at least have no problem adding it.

Should be able to handle this case just fine. Will try to get a PR together tomorrow.

Edit: #1680, when released, should correct both of your issues ("undefined" entries and assets with altered paths not being discovered)