Proposal: Break `<pf-icon>`'s javascript APIs in 4.0 for better webpack compatibility #2765

bennypowers · 2024-06-03T12:12:56Z

bennypowers
Jun 3, 2024
Maintainer

tl;dr: webpack has trouble with pf-icon and i think the only solution is a breaking change

PfIcon contains a dynamic import which references an expression spec:

patternfly-elements/elements/pf-icon/BaseIcon.ts

Lines 139 to 162 in 9702278

    
             protected async load() { 
        
               const { set, icon } = this; 
        
               const getter = this.#class.getters.get(set) ?? this.#class.getIconUrl; 
        
               let spec = 'UNKNOWN ICON'; 
        
               if (set && icon) { 
        
                 try { 
        
                   const gotten = getter(set, icon); 
        
                   if (gotten instanceof URL) { 
        
                     spec = gotten.pathname; 
        
                   } else { 
        
                     spec = gotten; 
        
                   } 
        
                   const mod = await import(spec); 
        
                   this.content = mod.default instanceof Node ? mod.default.cloneNode(true) : mod.default; 
        
                   await this.updateComplete; 
        
                   this.dispatchEvent(new Event('load', { bubbles: true })); 
        
                 } catch (error: unknown) { 
        
                   const event = new IconLoadError(spec, error as Error); 
        
                   this.#logger.error((error as IconLoadError).message); 
        
                   this.dispatchEvent(event); 
        
                 } 
        
               } 
        
             } 
        
           }

in particular line 151 there:

patternfly-elements/elements/pf-icon/BaseIcon.ts

Line 151 in 9702278

const mod = await import(spec);

This works fine in every runtime, but some bundlers (cough webpack) simply refuse to allow it.

We can fix this with a breaking change to pf-icon. I don't believe it's possible to do a "soft-deprecation" (i.e. to retain the existing apis but warn when they are used) because the existence of this import expression in the code in any form will cause this problem. the /* webpackIgnore */ comment does not help.

I've prepared some minimal reproductions (attached) which demonstrate that vite and rollup work fine, but webpack is unfortunately obstinate.

Vite 👍 Works

Reproduction

npm init vite -- --template vanilla vite-project
cd vite-project
npm i @patternfly/elements@3.0.1
echo "import '@patternfly/elements/pf-icon/pf-icon.js';" >> main.js
sed -i '4i <script type="importmap">{"imports":{"@patternfly/icons/":"/node_modules/@patternfly/icons/"}}</script>' index.html
sed -i '11i <pf-icon icon="ad"></pf-icon>' index.html
npm run dev

Rollup 👍 Works

Reproduction

git clone https://github.com/rollup/rollup-starter-app
cd rollup-starter-app
npm i @patternfly/elements@3.0.1
echo "import '@patternfly/elements/pf-icon/pf-icon.js';" >> src/main.js
sed -i '4i <script type="importmap">{"imports":{"@patternfly/icons/":"/@patternfly/icons/"}}</script>' public/index.html
sed -i '19i <pf-icon icon="ad"></pf-icon>' public/index.html
mkdir -p public/@patternfly
cp -r node_modules/@patternfly/icons public/@patternfly/
npm run build
npm start

Webpack 💣 Fails so hard

WARNING in ./node_modules/@patternfly/elements/pf-icon/BaseIcon.js 91:34-46
Critical dependency: the request of a dependency is an expression

Reproduction

npm init -y
npm i webpack webpack-cli webpack-dev-server @patternfly/elements@3
mkdir dist

echo '<!DOCTYPE html>
<html>
    <head>
        <title>Empty project</title>
        <meta charset="utf-8">
        <script type="importmap">{"imports":{"@patternfly/icons/":"/node_modules/@patternfly/icons/"}}</script>
    </head>
    <body>
        <pf-icon icon="ad"></pf-icon>
        <div id="app"></div>
        <script src="bundle.js"></script>
    </body>
</html>
' > dist/index.html
echo "import '@patternfly/elements/pf-icon/pf-icon.js';" > main.js
echo "const webpack = require('webpack');
const path = require('path');
module.exports = {
  entry: './main.js',
  devServer: {
    static: {
      directory: path.join(__dirname, 'dist'),
    },
  },
  output: {
    path: path.resolve(__dirname, 'dist'),
    filename: 'bundle.js'
  }
};" > webpack.config.js
npx webpack-cli serve --mode development

Proposal

I propose to remove the overridable static getIconUrl method and to change the signature of the static method addIconSet in a backwards-incompatible way.
I also propose adding a new overridable static resolve() method, similar to the old getIconUrl, which would return a promise of the icon node to render, rather than the url to import it from.

heyMP · 2024-06-03T12:24:54Z

heyMP
Jun 3, 2024
Maintainer

@bennypowers I think that's reasonable. Product Trials mostly relies on RHDC to load and configure PFE icon. There are a few instances were we reference the icon node directly in our components, but it sounds like the proposed changes won't impact that usage.

Example:

import * as InfoCircle from '@patternfly/elements/pf-icon/icons/fas/info-circle.js';

  renderTooltip() {
    return html`
      <rh-tooltip part="tooltip">
        ${InfoCircle.default.cloneNode(true)}
        <slot slot="content" name="authenticated-tooltip"></slot>
      </rh-tooltip>
    `;
  }

2 replies

bennypowers Jun 3, 2024
Maintainer Author

this snippet is anyways broken since we removed /icons in favour of @patternfly/icons instead

bennypowers Jun 3, 2024
Maintainer Author

one thing i think is worth considering is how we might act in an ssr scenario?

like, we'd ideally just print the icon in the shadow root in that case. lit/ssr support async rendering, but i'd want to be very confident we wouldn't need another breaking change to support it

eyevana · 2024-06-03T13:52:56Z

eyevana
Jun 3, 2024
Collaborator

The catalog uses the getIconUrl method so that icons are pulled from the CDN instead. How would we handle after the proposed change?

<script>
    (async function () {
      const PfIcon = await customElements.whenDefined("pf-icon");
      if (PfIcon) PfIcon.getIconUrl = (set, icon) =>
        `https://www.redhatstatic.com/dx/v1-alpha/@patternfly/icons@1.0.2/${set}/${icon}.js`;
    })();
</script>

1 reply

bennypowers Jun 3, 2024
Maintainer Author

I'm sure this would need improvement, but what about something like:

// assuming that the new API is called `resolve()`
if ('getIconUrl' in PfIcon) {
  PfIcon.getIconUrl = (set, icon) =>
    `https://www.redhatstatic.com/dx/v1-alpha/@patternfly/icons@1.0.2/${set}/${icon}.js`;
} else if ('resolve' in PfIcon) {
  PfIcon.resolve = async (set, icon) =>
    import(`https://www.redhatstatic.com/dx/v1-alpha/@patternfly/icons@1.0.2/${set}/${icon}.js`)
      .then(mod => mod.default);
}

BUT in this particular case, I think the best solution would just be to

<script type="importmap">
{ "imports": {
  "@patternfly/icons/": "https://www.redhatstatic.com/dx/v1-alpha/@patternfly/icons@1.0.2/"
} }
</script>

Which would work with both versions

bennypowers · 2024-06-03T14:26:15Z

bennypowers
Jun 3, 2024
Maintainer Author

@zhawkins

thanks!

1 reply

zhawkins Jun 3, 2024
Maintainer

yup, sorry. This is fine with us too 👍

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Break `<pf-icon>`'s javascript APIs in 4.0 for better webpack compatibility #2765

{{title}}

{{editor}}'s edit

{{editor}}'s edit

Replies: 3 comments 4 replies

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

Select a reply

Proposal: Break <pf-icon>'s javascript APIs in 4.0 for better webpack compatibility #2765

bennypowers Jun 3, 2024 Maintainer

Vite 👍 Works

Rollup 👍 Works

Webpack 💣 Fails so hard

Proposal

Replies: 3 comments · 4 replies

heyMP Jun 3, 2024 Maintainer

bennypowers Jun 3, 2024 Maintainer Author

bennypowers Jun 3, 2024 Maintainer Author

eyevana Jun 3, 2024 Collaborator

bennypowers Jun 3, 2024 Maintainer Author

bennypowers Jun 3, 2024 Maintainer Author

zhawkins Jun 3, 2024 Maintainer

Proposal: Break `<pf-icon>`'s javascript APIs in 4.0 for better webpack compatibility #2765

bennypowers
Jun 3, 2024
Maintainer

Replies: 3 comments 4 replies

heyMP
Jun 3, 2024
Maintainer

bennypowers Jun 3, 2024
Maintainer Author

bennypowers Jun 3, 2024
Maintainer Author

eyevana
Jun 3, 2024
Collaborator

bennypowers Jun 3, 2024
Maintainer Author

bennypowers
Jun 3, 2024
Maintainer Author

zhawkins Jun 3, 2024
Maintainer