[rollup-plugin-polyfills-loader] Possible race condition while writing polyfile scripts to disk.

kimtaa commented 1 year ago

Hi!

I'm having some trouble with getting the polyfill javascript files written to disk. They are only sometimes generated into the polyfills folder - more often if my system is under heavy load. So, I suspect a race condition of some sort.

Below is the config I use:

import { rollupPluginHTML as html } from "@web/rollup-plugin-html";
import {polyfillsLoader} from '@web/rollup-plugin-polyfills-loader';
import {copy} from '@web/rollup-plugin-copy';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import {getBabelOutputPlugin} from '@rollup/plugin-babel';
import terser from '@rollup/plugin-terser';
import pkgMinifyHTML from 'rollup-plugin-minify-html-literals';
const minifyHTML = pkgMinifyHTML.default
import summary from 'rollup-plugin-summary';

const htmlPlugin = html({
  rootDir: './build',
  flattenOutput: false,
  minify: false,
  transformHtml: [
    html => { // add csp nonce template tag as arg to polyfill loader script
      const regex = /(polyfills\.push\(loadScript\()(.*?)(\)\);)/g;
      return html.replace(regex, "$1 $2, null, [{\"name\": \"nonce\",\"value\": \"{{cspNonce}}\"}] $3");
    },
    html => html.replace(/<link rel=/g, "<link nonce=\"{{cspNonce}}\" rel="),
    html => html.replace(/<script>/g, "<script nonce=\"{{cspNonce}}\">")
  ]
});

export default {
  input: 'index.html',
  plugins: [
    htmlPlugin,
    nodeResolve(),
    minifyHTML(),
    terser({
      module: true,
      warnings: true,
    }),
    polyfillsLoader({
      modernOutput: {
        name: 'modern',
      },
      legacyOutput: { name: 'legacy', test: '!(\'noModule\' in HTMLScriptElement.prototype)' },
      polyfills: {
        hash: true,
        coreJs: true,
        regeneratorRuntime: true,
        fetch: true,
        webcomponents: true,
        custom: [
          {
            name: 'lit-polyfill-support',
            path: 'node_modules/lit/polyfill-support.js',
            test: "!('attachShadow' in Element.prototype)",
            module: false,
          },
        ],
      },
    }),
    summary(),
    copy({ rootDir: "build", patterns: "**/*.{svg,ttf,woff,woff2,eot}"}),
  ],
  output: [
    {
      format: 'esm',
      chunkFileNames: '[name]-[hash].js',
      entryFileNames: '[name]-[hash].js',
      dir: 'dist',
      plugins: [htmlPlugin.api.addOutput('modern')],
    },
    {
      preserveModules:false,
      dir: 'dist'
    },
    {
      format: 'esm',
      chunkFileNames: 'legacy-[name]-[hash].js',
      entryFileNames: 'legacy-[name]-[hash].js',
      dir: 'dist',
      plugins: [
        htmlPlugin.api.addOutput('legacy'),
        getBabelOutputPlugin({
          compact: true,
          presets: [
            [
              '@babel/preset-env',
              {
                targets: {
                  ie: '11',
                },
                modules: 'systemjs',
              },
            ],
          ],
        }),
      ],
    },
  ],

};

Any ideas or pointers on how to solve this?

Cheers!

thepassle commented 1 year ago

Any ideas or pointers on how to solve this?

I recommend creating a minimal reproduction, that usually highlights the issue pretty quickly.

kimtaa commented 1 year ago

Hi again!

Ok - I've created a minimal project, and either I'm hitting the bug all the time, or I've configured something wrong. The 'polyfills'-folder is not created no matter what I try. Could you try this out, and confirm whether or not the polyfills-folder is created for you?

index.html:

<!DOCTYPE html>
<html lang="en">
<head>
  <title>minimal</title>
  <script type="module" src="main-content.js"></script>
</head>
<body>
    <main-content></main-content>
</body>
</html>

main-content.ts:

import {LitElement, html} from 'lit';
import { customElement } from "lit/decorators.js";
@customElement('main-content')
export class MainContent extends LitElement {
  override render()  {
    return html`<p>test output</p>`;
  }
}

package.json:

{
  "name": "mini-test",
  "type": "module",
  "source": "index.html",
  "browserslist": [
    "> 0.1%, last 2 versions"
  ],
  "scripts": {
    "build": "yarn clean && tsc && rollup -c",
    "clean": "rm -rf dist/"
  },
  "dependencies": {
    "@lit-labs/context": "^0.4.0",
    "lit": "^2.2.4"
  },
  "devDependencies": {
    "@babel/core": "^7.0.0",
    "@babel/preset-env": "^7.22.15",
    "@rollup/plugin-babel": "^6.0.3",
    "@rollup/plugin-node-resolve": "^15.2.1",
    "@rollup/plugin-replace": "^5.0.2",
    "@rollup/plugin-terser": "^0.4.3",
    "@types/mocha": "^10.0.1",
    "@typescript-eslint/eslint-plugin": "^5.25.0",
    "@typescript-eslint/parser": "^5.25.0",
    "@web/polyfills-loader": "^2.1.2",
    "@web/rollup-plugin-copy": "^0.4.0",
    "@web/rollup-plugin-html": "^2.0.1",
    "@web/rollup-plugin-polyfills-loader": "^2.0.1",
    "@webcomponents/webcomponentsjs": "^2.6.0",
    "core-js": "^3.32.2",
    "custom-elements-es5-adapter": "^1.0.0",
    "eslint": "^8.15.0",
    "fetch": "^1.1.0",
    "regenerator-runtime": "^0.14.0",
    "rollup": "^3.29.1",
    "rollup-plugin-minify-html-literals": "^1.2.6",
    "rollup-plugin-summary": "^2.0.0",
    "typescript": "~4.7.4",
    "webcomponents": "^0.1.4"
  }
}

rollup.config.js:

import { rollupPluginHTML as html } from "@web/rollup-plugin-html";
import {polyfillsLoader} from '@web/rollup-plugin-polyfills-loader';

const htmlPlugin = html({ input: 'index.html' });
export default {
  onwarn(warning, warn) {
    if (warning.code === 'THIS_IS_UNDEFINED') return;
    warn(warning);
  },
  plugins: [
    htmlPlugin,
    polyfillsLoader({
      modernOutput: {name: 'modern',},
      legacyOutput: { name: 'legacy', test: '!(\'noModule\' in HTMLScriptElement.prototype)' },
      polyfills: {fetch: true},
    }),
  ],
  output: [
    {
      format: 'esm',
      chunkFileNames: '[name]-[hash].js',
      entryFileNames: '[name]-[hash].js',
      dir: './dist',
      plugins: [htmlPlugin.api.addOutput('modern')]
    },
    {
      format: 'esm',
      chunkFileNames: 'legacy-[name]-[hash].js',
      entryFileNames: 'legacy-[name]-[hash].js',
      dir: './dist',
      plugins: [htmlPlugin.api.addOutput('legacy')]
    },
  ],
};

tsconfig.json:

{
  "compilerOptions": {
    "target": "es2019",
    "module": "es2020",
    "lib": ["es2020", "DOM", "DOM.Iterable"],
    "declaration": true,
    "declarationMap": true,
    "sourceMap": true,
    "inlineSources": true,
    "outDir": "./",
    "rootDir": "./",
    "strict": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "noImplicitReturns": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitAny": true,
    "noImplicitThis": true,
    "moduleResolution": "node",
    "allowSyntheticDefaultImports": true,
    "experimentalDecorators": true,
    "forceConsistentCasingInFileNames": true,
    "noImplicitOverride": true,
    "allowJs": true,
    "isolatedModules": true,
    "plugins": [
      {
        "name": "ts-lit-plugin",
        "strict": true
      }
    ],
    "types": ["mocha"]
  },
  "include": ["*.ts"],
  "exclude": []
}

I was expecting yarn install && tsc && rollup -c to produce a 'polyfills' folder in 'dist'.

Cheers!

thepassle commented 1 year ago

There still seems to be quite a lot going on in the rollup config, can you try reducing that even more?

kimtaa commented 1 year ago

Yes - could've been smaller. I've updated the rollup config in the previous reply so that it's more basic now.

thepassle commented 1 year ago

We set generatedFiles in this callback that is being added to htmlPlugin.api.addHtmlTransformer:

https://github.com/modernweb-dev/web/blob/40fa569e1077ddad2fbca43a9ce4762c167f99a6/packages/rollup-plugin-polyfills-loader/src/rollupPluginPolyfillsLoader.ts#L45

When using multiple outputs, I see that in the generateBundle hook, generatedFiles is undefined: https://github.com/modernweb-dev/web/blob/40fa569e1077ddad2fbca43a9ce4762c167f99a6/packages/rollup-plugin-polyfills-loader/src/rollupPluginPolyfillsLoader.ts#L112

I tried to wrap the whole generateBundle in a setTimeout just for debugging purposes, and then I see that generatedFiles is defined:

async generateBundle(_, bundle) {
          setTimeout(() => {
            if (generatedFiles) {
              for (const file of generatedFiles) {
                // if the polyfills loader is used multiple times, this polyfill might already be output
                // so we guard against that. polyfills are already hashed, so there is no need to worry
                // about clashing
                if (!(file.path in bundle)) {
                  this.emitFile({
                    type: 'asset',
                    name: file.path,
                    fileName: file.path,
                    source: file.content,
                  });
                }
              }
            }
          })

        },

However, I still don't see them being output to dist/polyfills, even though I see the calls to this.emitFile happening. Im not really sure why 🤔

kimtaa commented 1 year ago

It appears as the html transformer is called after the generateBundle hook?

kimtaa commented 1 year ago

I've tried to follow the source code to understand what is happening.

Looks like the html-transformer that creates generatedFiles (added at rollupPluginPolyfillsLoader.buildStart) is called by the html-plugin as part of the output plugin. So, generatedFiles will always be empty when rollupPluginPolyfillsLoader.generateBundle is called.

Maybe the html-transformer could emit the files if the number of bundles are more than 1?

aigan commented 10 months ago

I created a test based on the ones in rollup-plugin-html.test.ts. Modules should keep their order. The tests are done without \n but I include them here for readability

<h1>Hello world</h1>
<script type="module">import "./entrypoint-a.js";</script>
<script type="module" src="./entrypoint-b.js"></script>

should output

<html><head></head><body><h1>Hello world</h1>
<script type="module" src="./inline-module-5ec680a4efbb48ae254268ab1defe610.js"></script>
<script type="module" src="./entrypoint-b.js"></script>
</body></html>

For multiple src there are a race condition, giving random order. I hope that this example with inline modules are related to that.

aigan commented 10 months ago

Here is the actual test i wrote for rollup-plugin-html.test.ts

  it('can resolve modules in original order', async () => {
    const config = {
      plugins: [
        rollupPluginHTML({
          rootDir,
          input: {
            name: 'index.html',
            html: '<h1>Hello world</h1>' +
              '<script type="module">import "./entrypoint-a.js";</script>' +
              '<script type="module" src="./entrypoint-b.js"></script>',
          },
        }),
      ],
    };

    const bundle = await rollup(config);
    const { output } = await bundle.generate(outputConfig);
    expect(output.length).to.equal(4);
    const hash = '5ec680a4efbb48ae254268ab1defe610';
    const { code: appCode } = getChunk(output, `inline-module-${hash}.js`);
    expect(appCode).to.include("console.log('entrypoint-a.js');");
    expect(stripNewlines(getAsset(output, 'index.html').source)).to.equal(
      '<html><head></head><body><h1>Hello world</h1>' +
        '<script type="module" src="./inline-module-5ec680a4efbb48ae254268ab1defe610.js"></script>' +
        '<script type="module" src="./entrypoint-b.js"></script>' +
        '</body></html>',
    );
  });

aigan commented 10 months ago

found the problem. Working on a patch.

modernweb-dev / web

[rollup-plugin-polyfills-loader] Possible race condition while writing polyfile scripts to disk. #2456