parcel icon indicating copy to clipboard operation
parcel copied to clipboard

Fix: remove pipeline on transforming svg to JSX

Open Shinyaigeek opened this issue 2 years ago • 3 comments

Fixes: #7587

(but NOT #6748 )

↪️ Pull Request

I noticed #7587 bug is caused because @parcel/transformer-svg-react transforms SVG image into React component by transforming code, change type from svg to jsx, but keep asset's pipeline.

https://parceljs.org/languages/svg/#importing-as-a-react-component

import SVGIcon from “jsx:./icon.svg”;

In above example, icon.svg asset is transformed into JSX asset which includes pipeline jsx by @parcel/transformer-svg-react, so any transformer does not match this asset after parcel/transformer-svg-react. (This is why it behaves as if it was resolved by jeb5’s workaround).

import SVGIcon from “jsx:./icon.jsx”; // any transformer does not match with default parcelrc 😢 

I came up with four-way to fix this.

  1. Fix parcelrc example in the docs about @parcel/transformer-svg-react with
{
  "extends": "@parcel/config-default",
  "transformers": {
    "jsx:*.svg": ["...", "@parcel/transformer-svg-react”],
    "jsx:*.jsx": [
      "@parcel/transformer-babel",
      "@parcel/transformer-js",
      "@parcel/transformer-react-refresh-wrap”
    ],
  }
}
  1. Fix parcelrc example in the docs about @parcel/transformer-svg-react with
{
  "extends": "@parcel/config-default",
  "transformers": {
    "*.svg": ["...", "@parcel/transformer-svg-react"],
  }
}
  1. fix the transformer matching behavior to match assets which includes some pipeline to transformers without pipeline as a fallback
  2. remove the pipeline info from Asset on transforming SVG to JSX.

1 is too bothering. 2 cannot cover all use cases (because we may handle some SVG images as a pure SVG though we handle some SVG images as a react component in the same application). I felt 3 will leads to unnecessary transforming and unexpected bugs or performance regression, so I choose 4.

💻 Examples

import Icon from "jsx:./icon.svg";

export const App = () => <Icon />;

✔️ PR Todo

  • [x] Added/updated unit tests for this change
  • [x] Filled out test instructions (In case there aren't any unit tests)
  • [x] Included links to related issues/PRs

Shinyaigeek avatar Feb 14 '22 18:02 Shinyaigeek

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

height[bot] avatar Feb 14 '22 18:02 height[bot]

I think this might also fix https://github.com/parcel-bundler/parcel/pull/7626 because the transform to commonjs probably shouldn’t be happening? Or maybe that’s specific to the svg use case

samjt avatar Feb 16 '22 13:02 samjt

Hi @samjt 👋

Though I may be misunderstood about the issue #7626 tries to resolve, I think this PR( or my proposal will not fix the issue, because this PR only tackles only about SVG to React Component transformation, and also my proposal only tackles about transpiring the module imported from node_modules. However, there might be something I missed.

Shinyaigeek avatar Feb 16 '22 15:02 Shinyaigeek