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(www): ensure that files necessary for www build are on disk #4992

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

alicewriteswrongs
Copy link
Contributor

This fixes an issue where the www build and, in particular, some code that attempts to inline the entry point for the www build into the index.html file that the build outputs, was depending on a file which, in certain circumstances, would not yet be built. This produced inconsistency across builds, where the www build would pick up and inline the entry point from the previous build (if the build wasn't deleted) instead of from the current build.

What is the current behavior?

fixes #4948

See the linked issue. Basically, with the www output target at present if you do

stencil build --dev --watch --serve

in a Stencil project and then you run a build (without deleting www/) like

stencil build

You'll get a non-working build. Not good!

Why does this happen? We have this function which is called as part of the www build:

export const optimizeEsmImport = async (
config: d.ValidatedConfig,
compilerCtx: d.CompilerCtx,
doc: Document,
outputTarget: d.OutputTargetWww,
) => {
const resourcesUrl = getAbsoluteBuildDir(outputTarget);
const entryFilename = `${config.fsNamespace}.esm.js`;
const expectedSrc = join(resourcesUrl, entryFilename);
const script = Array.from(doc.querySelectorAll('script')).find(
(s) =>
s.getAttribute('type') === 'module' && !s.hasAttribute('crossorigin') && s.getAttribute('src') === expectedSrc,
);
if (!script) {
return false;
}
script.setAttribute('data-resources-url', resourcesUrl);
script.setAttribute('data-stencil-namespace', config.fsNamespace);
const entryPath = join(outputTarget.buildDir, entryFilename);
const content = await compilerCtx.fs.readFile(entryPath);
if (isString(content)) {
// If the script is too big, instead of inlining, we hash the file and change
// the <script> to the new location
if (config.allowInlineScripts && content.length < MAX_JS_INLINE_SIZE) {
// Let's try to inline, we have to fix all the relative paths of the imports
const results = updateImportPaths(content, resourcesUrl);
if (results.orgImportPaths.length > 0) {
// insert inline script
script.removeAttribute('src');
script.innerHTML = results.code;
}
} else {
const hashedFile = await generateHashedCopy(config, compilerCtx, entryPath);
if (hashedFile) {
const hashedPath = join(resourcesUrl, hashedFile);
script.setAttribute('src', hashedPath);
injectModulePreloads(doc, [hashedPath]);
}
}
return true;
}
return false;
};
export const updateImportPaths = (code: string, newDir: string) => {
const orgImportPaths: string[] = [];
const tsSourceFile = ts.createSourceFile('module.ts', code, ts.ScriptTarget.Latest);
ts.transform(tsSourceFile, [readImportPaths(orgImportPaths)]);
orgImportPaths.forEach((orgImportPath) => {
const newPath = replacePathDir(orgImportPath, newDir);
if (newPath) {
code = code.replace(`"${orgImportPath}"`, `"${newPath}"`);
code = code.replace(`'${orgImportPath}'`, `'${newPath}'`);
}
});
return {
code,
orgImportPaths,
};
};
const replacePathDir = (orgImportPath: string, newDir: string) => {
if (orgImportPath.startsWith('./') && (orgImportPath.endsWith('.js') || orgImportPath.endsWith('.mjs'))) {
return newDir + orgImportPath.substring(2);
}
return null;
};
const readImportPaths = (orgImportPaths: string[]): ts.TransformerFactory<ts.SourceFile> => {
return () => {
return (tsSourceFile) => {
const importStatements = tsSourceFile.statements
.filter(ts.isImportDeclaration)
.filter((s) => s.moduleSpecifier != null)
.filter((s) => ts.isStringLiteral(s.moduleSpecifier) && s.moduleSpecifier.text);
importStatements.forEach((s) => {
if (ts.isStringLiteral(s.moduleSpecifier)) {
orgImportPaths.push(s.moduleSpecifier.text);
}
});
return tsSourceFile;
};
};
};
// https://twitter.com/addyosmani/status/1143938175926095872
const MAX_JS_INLINE_SIZE = 1 * 1024;

This will basically look for the entry point for the www build at www/build/project-name.js and, if it finds it, it will inline that code in the index.html file, reducing the number of JS files that have to be fetch on page-load by one.

However! Without this change we have a race condition where the lazy build (which writes the file we're looking for to disk) is not written until after the www build executes. This means that:

  • on the first build the entry point is not found, so nothing is inlined in the HTML. this produces a working build, but the optimization (inlining) is missed.
  • on the second build, if www/ is not deleted then the entry point from the first build is found and inlined or if the file is larger than a cutoff size then a hashed copy of it will be made and the <script> tag in index.html will be changed to point to the hashed copy.
    • in either case the result is that index.html will include the entry point for the previous build rather than for the current build and this can produce broken behavior, in particular because this might import other files that are no longer on the disk (and also because, you know, if you run a build you expect that the build emits code based on the current source code, not the source code the last time you ran a build)

Further, if both builds were 1) non-HMR / dev-server builds (i.e. the result of stencil build) then, provided there were no code changes between build one and build two, a working, inlined build will be the result. However! If either the first build was an HMR build (i.e. from stencil build --dev --watch --serve) or there was a code change so the hashed filenames will change then the file that is picked up will refer to things that don't exist. Not good!

What is the new behavior?

Now the www build is run separately from the other output targets after they all finish, to ensure that the files from the lazy build are written before the www build tries to read them.

Does this introduce a breaking change?

  • Yes
  • No

Testing

You can reproduce the issue by doing the following:

  1. Create a new stencil project:
    cd /tmp
    npm init stencil@latest component test-www-previous-build
    cd test-www-previous-build
    npm i
  2. Edit the stencil.config.ts to only do the www build (just for simplicity)
  3. start the project in watch mode:
    npx stencil build --dev --watch --serve
    This should work as normal
  4. Now do a normal build:
    npx stencil build
    this should exit without error
  5. Check the build output in the browser:
    npx http-server www
    This should not work, giving you some console errors about not resolving certain resources and whatnot.

If you build, pack, and install this branch the issue should be fixed.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2023

--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 14 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/utils/index.ts 232 resolve
src/utils/index.ts 246 normalize
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

Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

Interestingly, https://github.com/ionic-team/stencil/pull/2970/files makes the same change (sans some comment differences). I tried to get #2970 working (since we hadn't touched it in a while) and couldn't - nice!

We can probably close #2970 out if this works out :-)

@alicewriteswrongs
Copy link
Contributor Author

Ah so it does! Yeah I agree we can probably close. Also for future reference the inlining behavior can be disabled

/**
* By default, Stencil will attempt to optimize small scripts by inlining them in HTML. Setting
* this flag to `false` will prevent this optimization and keep all scripts separate from HTML.
*/
allowInlineScripts?: boolean;

@alicewriteswrongs
Copy link
Contributor Author

I think I broke a karma test, investigating that this morning

@rwaskiewicz
Copy link
Member

@alicewriteswrongs FWIW, I think that's the problem I ran into myself. I don't have much to offer beyond that 😭 but LMK if you wanna sync up - I was hacking on this when the team was in Madison earlier this year, and never root caused the issue

This fixes an issue where the `www` build and, in particular, some code
that attempts to inline the entry point for the `www` build into the
`index.html` file that the build outputs, was depending on a file which,
in certain circumstances, would not yet be built. This produced
inconsistency across builds, where the `www` build would pick up and
inline the entry point from the _previous_ build (if the build wasn't
deleted) instead of from the current build.

To go into more detail, prior to this change with the `www` output
target if you did:

```sh
stencil build --dev --watch --serve
```

in a Stencil project and _then_ you ran a build (without deleting
`www/`) like:

```sh
stencil build
```

You'd get a non-working build. Not good!

Why did this happen? We have this function which is called as part of
the `www` build:

https://github.com/ionic-team/stencil/blob/5c4ea3d5354596e5cfb544f91bde7f61de1888b4/src/compiler/html/inline-esm-import.ts#L10-L105

This will basically look for the entry point for the `www` build at
`www/build/project-name.js` and, if it finds it, it will inline that
code in the `index.html` file, reducing the number of JS files that have
to be fetch on page-load by one.

However! Without this change we have a race condition where the lazy
build (which writes the file we're looking for to disk) is not written
until after the `www` build executes. This means that:

- on the first build the entry point is not found, so nothing is inlined
  in the HTML. this produces a working build, but the optimization
  (inlining) is missed.
- on the second build, if `www/` is not deleted then the entry point
  from the _first_ build is found and inlined _or_ if the file is larger
  than a cutoff size then a hashed copy of it will be made and the
  `<script>` tag in `index.html` will be changed to point to the hashed
  copy.
    - in either case the result is that `index.html` will include the
      entry point _for the previous build_ rather than for the _current_
      build and this can produce broken behavior, in particular because
      this might import _other_ files that are no longer on the disk
      (and also because, you know, if you run a build you expect that
      the build emits code based on the current source code, not the
      source code the last time you ran a build)

Further, if both builds were 1) non-HMR / dev-server builds (i.e. the
result of `stencil build`) then, provided there were no code changes
between build one and build two, a working, inlined build will be the
result. However! If _either_ the first build was an HMR build (i.e. from
`stencil build --dev --watch --serve`) _or_ there was a code change so
the hashed filenames will change then the file that is picked up will
refer to things that don't exist. Not good!

fixes #2687
fixes #4948
closes #2970
@alicewriteswrongs alicewriteswrongs added this pull request to the merge queue Oct 27, 2023
Merged via the queue into main with commit b74220b Oct 27, 2023
120 checks passed
@alicewriteswrongs alicewriteswrongs deleted the ap/dev-then-prod-www branch October 27, 2023 18:46
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.

bug: Stencil first prod build is broken after dev build
3 participants