Skip to content

Build ESM views with external CDN support#1382

Closed
cristiano-belloni wants to merge 110 commits intorelease/v4from
feature/module-rewrite
Closed

Build ESM views with external CDN support#1382
cristiano-belloni wants to merge 110 commits intorelease/v4from
feature/module-rewrite

Conversation

@cristiano-belloni
Copy link
Contributor

@cristiano-belloni cristiano-belloni commented Feb 8, 2022

This is related to #1311

  • Build views as ESM modules with external CDN
  • Build a synthetic index.html and _trampoline.js to distribute views as static site (modular serve view works)
  • Support many CDNs with environment templates - for example, EXTERNAL_CDN_TEMPLATE="https://esm.sh/[name]@[version]" for esm.sh, EXTERNAL_CDN_TEMPLATE="https://cdn.skypack.dev/[name]@[version]" for Skypack. Default is Skypack.
  • Support blocklist for external dependencies (can be transformed into allowlist for gradual introduction)
  • Set module field in output package.json with the built (hashed) module name.
  • Set correct bundledDependencies array.
  • view serve works for views
  • Use css module scripts for esnext targets and fallback to link css injection for global css imports from the CDN.
  • Fix openBrowser.ts to actually work on CI (wrong default value to satisfy type was forcing it to open OSX Chrome)
  • Test all functionalities
  • Only support esbuild for now - for webpack, we need Webpack 5 - a PR for WP5 support is here: Build ESM views with external CDN support - esbuild & webpack 5 #1439

@changeset-bot
Copy link

changeset-bot bot commented Feb 8, 2022

🦋 Changeset detected

Latest commit: 78bc31e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
modular-scripts Patch
create-modular-react-app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cristiano-belloni cristiano-belloni changed the title Generate ESM views with external CDN support Build ESM views with external CDN support Feb 8, 2022
};
}
const externalBlockList =
process.env.EXTERNAL_BLOCK_LIST && !isApp
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we choose to block things? I think exposing this API without an understanding of why this is necessary is probably a bit careless.

Copy link
Contributor Author

@cristiano-belloni cristiano-belloni Feb 18, 2022

Choose a reason for hiding this comment

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

Mainly for three reasons:

  1. If you have an organisation with a private registry, some internal packages could be present in the registry but not in the (possibly public) ESM CDN. In that case, you will need to bundle them.
  2. You could be importing a dependency in the monorepo that "looks" like an external dep from the import statement but in reality is resolved by workspace symlinks (and it's not meant to be published to the registry or the CDN). In that case you almost always want to bundle it (we could filter out the workspace dependencies by default, but then you have the inverse problem: what if they're separately published to the CDN and you want to rewrite them?)
  3. You could want an escape hatch for when a dependency is broken in the CDN and you can't wait for the CDN to fix it. So you decide to be a bit less efficient and bundle it, until the CDN fixes it, you remove the dependency from the blocklist, re-bundle and redeploy.

We can also have allow lists - they'd be useful in case the CDN exports only a selected range of dependencies (although less useful IMHO). But I wouldn't remove block lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(or maybe you meant that EXTERNAL_BLOCK_LIST is a misnomer? It contains an optional list of dependencies that are to not be rewritten to an external CDN, but bundled in the build file. If you have a better name, happy to change it)

// If you want to use webpack then we'll always use webpack. But if you've indicated
// you want esbuild - then we'll switch you to the new fancy world.
if (!useWebpack || useEsbuild) {
if (!useWebpack || useEsbuild || isView) {
Copy link
Contributor

@LukeSheard LukeSheard Feb 18, 2022

Choose a reason for hiding this comment

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

This is untransparent for people that are still using webpack for apps they will probably be expecting webpack for starting a view. I think it would be better to throw an error down the webpack path if it's a view so that people are blocked from that flow, given esbuild is still opt in, and we do currently support webpack for starting views.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. This is going to be temporary until we have Webpack 5, but I will log an error and throw down the webpack path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, in start and build too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another PR that supports Webpack 5 and contains this PR. For the sake of simplicity, I'm keeping them separated.

dependencies: Dependency,
browserTarget: string[],
): Promise<esbuild.BuildResult & { outputFiles: esbuild.OutputFile[] }> {
const fileRelativePath = `./${fileName}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely this would be the package name?

Copy link
Contributor Author

@cristiano-belloni cristiano-belloni Feb 18, 2022

Choose a reason for hiding this comment

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

This is index.js if we're starting a view or index-[HASH].js if we're building a view. I think that the view entrypoint js file should be hashed for cache-breaking purposes? After all whoever loads it will fetch it at runtime. We keep track of the name in the output package.json, module field, so there is always a way to get the hashed file name. If you think it should be named differently let's have a chat about that.

@cristiano-belloni cristiano-belloni changed the base branch from main to release/esm-cdn March 15, 2022 13:49
@cristiano-belloni cristiano-belloni changed the base branch from release/esm-cdn to release/v4 April 6, 2022 10:54
@cristiano-belloni
Copy link
Contributor Author

Close this as it's contained in #1439

@LukeSheard LukeSheard deleted the feature/module-rewrite branch April 11, 2022 16:30
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.

3 participants