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: Modify manifest entries for asset entrypoints (close #1765) #1774

Closed
wants to merge 1 commit into from

Conversation

ElMassimo
Copy link
Contributor

@ElMassimo ElMassimo commented Jan 28, 2021

Description 📖

This pull request is an attempt to fix #1765, by modifying the manifest output for asset entrypoints.

The manifest output now ignores empty chunks, and replaces their file with an actual asset path (if one exists).

As a result, the regression is fixed, and obtaining the resource URL for generic entries (JS/CSS/images) becomes simpler since file is always the source of truth for the resource path, regardless of the entry type.

Before

  "logo.svg": {
    "file": "assets/logo.f1745d22.js",
    "src": "logo.svg",
    "isEntry": true
  }

But the fingerprinted file is actually assets/logo.41cff365.svg, which is not in the manifest, but is in the output dir.

After

  "logo.svg": {
    "file": "assets/logo.41cff365.svg",
    "src": "logo.svg",
    "isEntry": true
  }

Background 📜

When adding a non-JS and non-CSS entrypoint, such as an .svg, in the manifest entry:

  • A fingerprinted version of the file is added correctly to the output dir, but
  • file points to an empty JS chunk, and it can't be used to resolve the file

Prior to beta 51 file had the resolved path to the fingerprinted file.

Manifest in Beta 51/52
{
  "application.js": {
    "file": "assets/application.d9514acc.js",
    "src": "application.js",
    "isEntry": true,
    "imports": [
      "_vendor.880705da.js",
      "example_import.js"
    ],
    "css": "assets/application.f510c1e9.css"
  },
  "example_import.js": {
    "file": "assets/example_import.8e1fddc0.js",
    "src": "example_import.js",
    "isEntry": true
  },
  "logo.svg": {
    "file": "assets/logo.f1745d22.js",
    "src": "logo.svg",
    "isEntry": true
  },
  "styles.css": {
    "file": "assets/styles.81b3e972.js",
    "src": "styles.css",
    "isEntry": true,
    "css": "assets/styles.0e53e684.css"
  },
  "_vendor.880705da.js": {
    "file": "assets/vendor.880705da.js"
  }
}
Entire Manifest after changes in this PR
{
  "application.js": {
    "file": "assets/application.d9514acc.js",
    "src": "application.js",
    "isEntry": true,
    "imports": [
      "_vendor.880705da.js",
      "example_import.js"
    ],
    "css": "assets/application.f510c1e9.css"
  },
  "example_import.js": {
    "file": "assets/example_import.8e1fddc0.js",
    "src": "example_import.js",
    "isEntry": true
  },
  "logo.svg": {
    "file": "assets/logo.41cff365.svg",
    "src": "logo.svg",
    "isEntry": true
  },
  "styles.css": {
    "file": "assets/styles.0e53e684.css",
    "src": "styles.css",
    "isEntry": true
  },
  "_vendor.880705da.js": {
    "file": "assets/vendor.880705da.js"
  }
}

Have in mind that it doesn't affect the format for non-entrypoint CSS in the manifest.

)
if (asset) chunk.fileName = asset.fileName
return chunk
}
Copy link
Contributor Author

@ElMassimo ElMassimo Jan 28, 2021

Choose a reason for hiding this comment

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

Didn't want to optimize it until we discuss the approach.

We should use an approach similar to chunkToEmittedAssetsMap, where we index the corresponding asset in a Map as we process the chunks, so that we can efficiently retrieve them here.

An alternative is to simply replace the fileName in the chunk beforehand, but I'm not sure if the processing order would allow that.

Copy link
Member

Choose a reason for hiding this comment

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

Since multiple chunks can have the same name, this can result in wrong mapping if two entry assets have the same name. e.g. logo.svg and logo.png

Copy link
Contributor Author

@ElMassimo ElMassimo Jan 28, 2021

Choose a reason for hiding this comment

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

In this case we would be comparing resolved names, which include the file extension, and entrypoint assets always include the file extension in their name, so there wouldn't be a collision, correct?

@@ -19,6 +19,10 @@ interface ManifestChunk {
dynamicImports?: string[]
}

function isEmptyChunk(chunk: OutputChunk): boolean {
return !chunk.code || (chunk.code.length <= 1 && !chunk.code.trim())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the cases I observed, code was always \n, so there's potential for simplification here.

Didn't want to run trim() on a large string, hence the check for length.

@yyx990803
Copy link
Member

See #1765 (comment) - I believe it's better suited as a custom plugin instead of handling this in the manifest, which strictly assumes entries are js chunks and assets are just emitted files that belongs to chunks.

@yyx990803 yyx990803 closed this Jan 28, 2021
@ElMassimo ElMassimo deleted the feat-assets-manifest branch January 28, 2021 22:42
@ElMassimo
Copy link
Contributor Author

strictly assumes entries are js chunks

Gotcha, wasn't aware of this design decision, since the manifest listed asset entrypoints prior to beta 51.

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.

Regression (beta 51) - Manifest entries for SVG assets lack information to resolve the file
2 participants