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

fix: push manifest #1680

Merged
merged 4 commits into from Mar 29, 2022
Merged

fix: push manifest #1680

merged 4 commits into from Mar 29, 2022

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Mar 23, 2022

What kind of change does this PR introduce?

Bug fix / refactoring for future use

Did you add tests for your changes?

Yes

Summary

Closes #1675

In short, the push-manifest.json output had hard-coded expectations that caused two problems in the linked issue:

  • If the asset could not be found, there would be an "undefined" entry in the push-manifest
  • If the asset output paths were altered, they could not be added to the manifest due to the strict filename matching

This fixes both issues by using webpack-manifest-plugin (Webpack really should make this part of core, extremely useful to expose this info to other plugins).

This plugin provides a map output assets that allows us to keep our expectation about a bundle.css or route-home.js existing, but allows users to still customize the actual output. For example, the following config:

// preact.config.js
export default (config, env, helpers) => {
	config.output.publicPath = '/foo/';

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

...would create the following manifest:

{
  "/bundle.css": "/foo/styles/foobar.css",
  "/bundle.js": "/foo/bundle.29bec.esm.js",
  "/bundle.js.map": "/foo/bundle.29bec.esm.js.map",
}

Should allow users to customize output names as they wish without it interfering with any assumptions we've made about output.

In the future we might be able to make greater use of this and move away from asset path concatenation like the following:

<link rel="manifest" href="<%= htmlWebpackPlugin.files.publicPath %>manifest.json">

However, for now, I'd hesitate to do that as this might break some configs. Better to wait for a major.

Still, it should allow for much more robust targeting of output assets as it won't fall to pieces if a user makes configuration changes.

Does this PR introduce a breaking change?

No

@rschristian rschristian requested a review from a team as a code owner March 23, 2022 00:21
@changeset-bot
Copy link

changeset-bot bot commented Mar 23, 2022

🦋 Changeset detected

Latest commit: 86338c6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rschristian rschristian changed the title Fix/asset manifest Fix/push manifest Mar 23, 2022
Comment on lines -1 to -2
const isESM = filename => /\.esm\.js$/.test(filename);
const isMatch = (filename, condition) => isESM(filename) === condition;
Copy link
Member Author

Choose a reason for hiding this comment

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

In case this raises an eyebrow, the asset manifest will always map to ESM output if enabled, CJS if it's not. We don't need to deal with that ourselves anymore.

There's a new test to ensure the push-manifest.json is correct and referring to ESM scripts when ESM is enabled.

@ForsakenHarmony ForsakenHarmony changed the title Fix/push manifest fix: push manifest Mar 29, 2022
@ForsakenHarmony ForsakenHarmony enabled auto-merge (squash) March 29, 2022 15:25
@ForsakenHarmony ForsakenHarmony merged commit fcd0375 into master Mar 29, 2022
@ForsakenHarmony ForsakenHarmony deleted the fix/asset-manifest branch March 29, 2022 15:29
@preact-bot preact-bot mentioned this pull request Mar 29, 2022
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

Successfully merging this pull request may close these issues.

Push manifest does not adhere custom changes in preact config file
2 participants