fix(www): fix an inconsistency between www builds #4983

alicewriteswrongs · 2023-10-25T17:26:24Z

This fixes a bug where subsequent www builds would not always produce working output. In particular, prior to this change a 'clean' www build (i.e one done after deleting all previous output) would produce a working build but subsequent builds would not.

This had to do with an optimization that Stencil attempts to do during the www build where it will attempt to inline the JS code for the main entry point into the index.html that it produces.

As part of doing this it needs to rewrite some relative paths for any module specifiers that are part of that entry point, since the HTML file is found at www/index.html while the JS entry point will be typically found at www/build/project-name.js. Thus if that entry point has an import like import foo from './bar.js' that will need to be rewritten to import foo from '/build/bar.js' in order for the import to resolve once its inlined into the HTML.

This inlining optimization was previously broken because the path rewrite code did not account for the presence of more than one instance of a given module specifier in the soon-to-be-inlined code. However, by default we have both import and export declarations in that code, so the inlined code would end up non-functional.

To give an example, the problematic excerpt of the inlined code would look typically like this:

import {
  p as e,
  b as t
} from "/build/p-71aeae62.js";
export {
  s as setNonce
}
from "./p-71aeae62.js";

so the first import declaration is being correctly rewritten to begin with /build/ but the export declaration is not. The code should look like this:

import {
  p as e,
  b as t
} from "/build/p-71aeae62.js";
export {
  s as setNonce
}
from "/build/p-71aeae62.js";

This commit fixes the issue by transforming all instances of a given module specifier in the text, and additionally adds a regression test that tests for this specific case.

What is the current behavior?

You can reproduce the issue by doing the following:

Create a new stencil project:

cd /tmp
npm init stencil@latest component test-www-double-build
cd test-www-double-build
npm i

Edit the stencil.config.ts to only do the www build (just for simplicity)
build the project:
```
npx stencil build
```

Observe that build/index.html. It should look something like this:

<!doctype html>
<html dir="ltr" lang="en">
   <head>
      <meta charset="utf-8">
      <meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=5.0">
      <title>Stencil Component Starter</title>
      <script type="module" src="/build/test-dev-build-reload.esm.js" data-stencil data-resources-url="/build/" data-stencil-namespace="test-dev-build-reload"></script> <script nomodule="" src="/build/test-dev-build-reload.js" data-stencil>. 
      </script> 
   </head>
   <body>
      <my-component first="Stencil" last="'Don't call me a framework' JS"></my-component>
   </body>
</html>

Try this build output in a browser. The easiest way is to use a simple webserver to serve the www directory locally, for instance:
```
npx http-server www
```
This should work with no errors in the console and the <my-component> tag should work properly.

Rebuild the project without deleting or modifying anything in www/ and check out build/index.html again. It should now have inlined JS in the <script> tag, like so:

<!doctype html>
<html dir="ltr" lang="en">
   <head>
      <meta charset="utf-8">
      <meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=5.0">
      <title>Stencil Component Starter</title>
      <script type="module" data-stencil data-resources-url="/build/" data-stencil-namespace="test-dev-build-reload">
      import{p as e,b as t}from"/build/p-71aeae62.js";export{s as setNonce}from"./p-71aeae62.js";const o=()=>{const s=import.meta.url;const t={};if(s!==""){t.resourcesUrl=new URL(".",s).href}return e(t)};o().then((e=>t([["p-a9942e82",[[1,"my-component",{first:[1],middle:[1],last:[1]}]]]],e)));
         //# sourceMappingURL=test-dev-build-reload.esm.js.map
      </script> <script nomodule="" src="/build/test-dev-build-reload.js" data-stencil></script> 
   </head>
   <body>
      <my-component first="Stencil" last="'Don't call me a framework' JS"></my-component>
   </body>
</html>

Now try out this build output in the browser (as above). It should fail because the ./p-71aeae62.js module specifier can't be resolved (it should be /build/p-71aeae62.js instead).

What is the new behavior?

If you build and install this in the same project as above you should be able to rebuild without producing a non-working build.

Does this introduce a breaking change?

Yes
No

Testing

See above. I also expanded the unit tests for this bit of code a bit.

github-actions · 2023-10-25T17:28:24Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1393 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/mock-doc/serialize-node.ts	36
src/dev-server/server-process.ts	32
src/compiler/build/build-stats.ts	27
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts	25
src/compiler/style/test/optimize-css.spec.ts	23
src/testing/puppeteer/puppeteer-element.ts	23
src/compiler/prerender/prerender-main.ts	22
src/runtime/vdom/vdom-render.ts	20
src/runtime/client-hydrate.ts	19
src/screenshot/connector-base.ts	19
src/compiler/config/test/validate-paths.spec.ts	16
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/compiler/transpile/transpile-module.ts	14
src/runtime/vdom/vdom-annotations.ts	14
src/sys/node/node-sys.ts	14
src/compiler/build/build-finish.ts	13
src/compiler/prerender/prerender-queue.ts	13

Our most common errors

Typescript Error Code	Count
TS2345	418
TS2322	398
TS18048	310
TS18047	100
TS2722	38
TS2532	34
TS2531	23
TS2454	14
TS2352	13
TS2769	10
TS2790	10
TS2538	8
TS2344	5
TS2416	4
TS2493	3
TS18046	2
TS2684	1
TS2488	1
TS2430	1

Unused exports report

There are 12 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports

File	Line	Identifier
src/runtime/bootstrap-lazy.ts	21	setNonce
src/screenshot/screenshot-fs.ts	18	readScreenshotData
src/testing/testing-utils.ts	198	withSilentWarn
src/utils/index.ts	145	CUSTOM
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	115	Env
src/compiler/app-core/app-data.ts	117	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	62	satisfies
src/compiler/types/validate-primary-package-output-target.ts	62	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

alicewriteswrongs

some notes

alicewriteswrongs · 2023-10-25T21:20:15Z

src/compiler/html/inline-esm-import.ts

-  const orgImportPaths: string[] = [];
-  const tsSourceFile = ts.createSourceFile('module.ts', code, ts.ScriptTarget.Latest);
-  ts.transform(tsSourceFile, [readImportPaths(orgImportPaths)]);
+  const orgModulePaths = readModulePaths(code);


I made a change here because I don't think it's actually necessary to use ts.transform here, we were doing so purely to gather information about the source code, not to actually run a transformer and then emit code from it. So instead we can just create a ts.SourceFile and then iterate through the statements on it

Agreed - I can't think of/see a reason to use transform here 🤔

I thought about doing the lean-more-into-TypeScript refactor to actually emit the code but I think that's a lot more overhead actually

alicewriteswrongs · 2023-10-25T21:21:52Z

src/compiler/html/inline-esm-import.ts

+function isImportOrExportDecl(stmt: ts.Statement): stmt is ts.ImportDeclaration | ts.ExportDeclaration {
+  return ts.isExportDeclaration(stmt) || ts.isImportDeclaration(stmt);
+}


this is I think necessary because I don't think TS is smart enough (yet!) to know that ts.isExportDeclaration(foo) || ts.isImportDeclaration(foo) evaluating to true means it must be one of those two types - instead when I inlined the usage of these two functions it was inferring a type of ts.Statement :/

alicewriteswrongs · 2023-10-25T21:22:27Z

src/compiler/html/test/update-esm-import-paths.spec.ts

+  it('should update both an import and an export statement', () => {
+    const input = `import './p-010101.js'; export { foo } from './p-010101.js'`;
+    const o = updateImportPaths(input, newAbsDir);
+    expect(o.code).toBe(`import '/build/p-010101.js'; export { foo } from '/build/p-010101.js'`);
+  });


This is a regression test for the bug, I confirmed that it fails on main

tanner-reits

One possible change, but not a blocker

tanner-reits · 2023-10-26T18:14:53Z

src/compiler/html/inline-esm-import.ts

+      code = code.replace(new RegExp(`"${orgImportPath}"`, 'g'), `"${newPath}"`);
+      code = code.replace(new RegExp(`'${orgImportPath}'`, 'g'), `'${newPath}'`);


Do we need to keep the same quotation style that the file originally had? Otherwise we could instantiate a single regex to match the import with either quotation style

yeah I suppose we don't - I kept the same quotation-style-preserving logic that was already present, but I really just did so because that was the precedent and not for any real reason

Yeah, I don't think we need it, but I'm not opposed to keeping it either. The only thing I can think if is that different quote styles can be used to allow another quote style to be used in a string:

const foo = "We are 'the' champions";

which I don't think would be allowed in module paths (otherwise that'd be very confusing for users!)

ok maybe out of an abundance of caution I'll just leave it as-is for now

rwaskiewicz

LGTM - I had one tiny ask that's not blocking

rwaskiewicz · 2023-10-27T14:08:57Z

src/compiler/html/inline-esm-import.ts

+ * @param code the code to transform
+ * @returns a list of the module specifiers present in the code
+ */
+function readModulePaths(code: string) {


can we throw a return type on this function please?

rwaskiewicz · 2023-10-27T14:09:33Z

src/compiler/html/inline-esm-import.ts

-  const orgImportPaths: string[] = [];
-  const tsSourceFile = ts.createSourceFile('module.ts', code, ts.ScriptTarget.Latest);
-  ts.transform(tsSourceFile, [readImportPaths(orgImportPaths)]);
+  const orgModulePaths = readModulePaths(code);


Agreed - I can't think of/see a reason to use transform here 🤔

rwaskiewicz · 2023-10-27T14:12:30Z

src/compiler/html/inline-esm-import.ts

+      code = code.replace(new RegExp(`"${orgImportPath}"`, 'g'), `"${newPath}"`);
+      code = code.replace(new RegExp(`'${orgImportPath}'`, 'g'), `'${newPath}'`);


Yeah, I don't think we need it, but I'm not opposed to keeping it either. The only thing I can think if is that different quote styles can be used to allow another quote style to be used in a string:

const foo = "We are 'the' champions";

which I don't think would be allowed in module paths (otherwise that'd be very confusing for users!)

This fixes a bug where subsequent `www` builds would not always produce working output. In particular, prior to this change a 'clean' `www` build (i.e one done after deleting all previous output) would produce a working build but subsequent builds would not. This had to do with an optimization that Stencil attempts to do during the `www` build where it will attempt to inline the JS code for the main entry point into the `index.html` that it produces. As part of doing this it needs to rewrite some relative paths for any module specifiers that are part of that entry point, since the HTML file is found at `www/index.html` while the JS entry point will be typically found at `www/build/project-name.js`. Thus if that entry point has an import like `import foo from './bar.js'` that will need to be rewritten to `import foo from '/build/bar.js'` in order for the import to resolve once its inlined into the HTML. This inlining optimization was previously broken because the path rewrite code did not account for the presence of more than one instance of a given module specifier in the soon-to-be-inlined code. However, by default we have both `import` and `export` declarations in that code, so the inlined code would end up non-functional. To give an example, the problematic excerpt of the inlined code would look typically like this: ```js import { p as e, b as t } from "/build/p-71aeae62.js"; export { s as setNonce } from "./p-71aeae62.js"; ``` so the first `import` declaration is being correctly rewritten to begin with `/build/` but the `export` declaration is not. The code should look like this: ```js import { p as e, b as t } from "/build/p-71aeae62.js"; export { s as setNonce } from "/build/p-71aeae62.js"; ``` This commit fixes the issue by transforming all instances of a given module specifier in the text, and additionally adds a regression test that tests for this specific case.

alicewriteswrongs force-pushed the ap/www-rebuild-huh branch from 76cb26e to 383bfe5 Compare October 25, 2023 18:23

alicewriteswrongs marked this pull request as ready for review October 25, 2023 21:15

alicewriteswrongs requested a review from a team as a code owner October 25, 2023 21:15

alicewriteswrongs requested review from rwaskiewicz and tanner-reits October 25, 2023 21:15

alicewriteswrongs commented Oct 25, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/www-rebuild-huh branch from 383bfe5 to 698e2c8 Compare October 25, 2023 21:23

tanner-reits approved these changes Oct 26, 2023

View reviewed changes

rwaskiewicz approved these changes Oct 27, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/www-rebuild-huh branch from 698e2c8 to bf86233 Compare October 27, 2023 14:46

alicewriteswrongs enabled auto-merge October 27, 2023 14:46

alicewriteswrongs force-pushed the ap/www-rebuild-huh branch from bf86233 to 5e28022 Compare October 27, 2023 14:50

alicewriteswrongs added this pull request to the merge queue Oct 27, 2023

Merged via the queue into main with commit f113b05 Oct 27, 2023
119 of 120 checks passed

alicewriteswrongs deleted the ap/www-rebuild-huh branch October 27, 2023 15:18

rwaskiewicz added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(www): fix an inconsistency between www builds #4983

fix(www): fix an inconsistency between www builds #4983

alicewriteswrongs commented Oct 25, 2023

github-actions bot commented Oct 25, 2023 •

edited

Loading

alicewriteswrongs left a comment

alicewriteswrongs Oct 25, 2023

rwaskiewicz Oct 27, 2023

alicewriteswrongs Oct 27, 2023

alicewriteswrongs Oct 25, 2023

alicewriteswrongs Oct 25, 2023

tanner-reits left a comment

tanner-reits Oct 26, 2023

alicewriteswrongs Oct 26, 2023

rwaskiewicz Oct 27, 2023

alicewriteswrongs Oct 27, 2023

rwaskiewicz left a comment

rwaskiewicz Oct 27, 2023

rwaskiewicz Oct 27, 2023

rwaskiewicz Oct 27, 2023

		code = code.replace(new RegExp(`"${orgImportPath}"`, 'g'), `"${newPath}"`);
		code = code.replace(new RegExp(`'${orgImportPath}'`, 'g'), `'${newPath}'`);

fix(www): fix an inconsistency between www builds #4983

fix(www): fix an inconsistency between www builds #4983

Conversation

alicewriteswrongs commented Oct 25, 2023

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

github-actions bot commented Oct 25, 2023 • edited Loading

--strictNullChecks error report

reports and statistics

Unused exports report

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner-reits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2023 •

edited

Loading

`--strictNullChecks` error report