Feature/esm views #1686

cristiano-belloni · 2022-05-03T17:12:59Z

Introduce new type esm-view, which builds like app (both webpack & esbuild), but:

Has a single entrypoint exporting something (typically a React component)
Builds an ES Module
Rewrites dependencies to native imports to a CDN URL (which dependencies and which CDN is configurable)
Can specify which dependencies are rewritten and which ones are bundled with an allow list and block list env variable
Creates a synthetic trampoline + index.html to run standalone - this behaviour assumes we're building a React component (as per default template), we might have different configuration options in the future

See the doc PR (WIP) for more detailed info: #1704

…bpack-5

coveralls · 2022-05-05T10:44:59Z

Coverage decreased (-1.4%) to 25.172% when pulling 925b5a1 on feature/esm-views into 50d456c on main.

steveukx · 2022-05-09T11:47:59Z

packages/modular-scripts/react-scripts/config/webpack.config.js

-  ? JSON.parse(process.env.ESBUILD_TARGET_FACTORY)
-  : 'es2015';
+const isApp = process.env.MODULAR_IS_APP === 'true';
+const isEsmView = !isApp;


I was originally going to ask:

Is it safe to rely on process.env.MODULAR_IS_APP !== 'true' here? For my reference... where do the MODULAR_IS_APP / ESBUILD_TARGET_FACTORY environment variables come from? Are they set by the user or by reading configuration from a file etc?

having read further down I see these are set by code in the build. Might it be useful to have an environment variable to define the output type here (ie: ESM vs compiled)?

having read further down I see these are set by code in the build. Might it be useful to have an environment variable to define the output type here (ie: ESM vs compiled)?

Not sure I understand, but if we are running Webpack we are sure that we're either building an app or an esm-view. Defining MODULAR_COMPILE_TARGET='esm' | 'compile' (for example) will be functionally equivalent (isApp = MODULAR_COMPILE_TARGET === 'compile') but maybe a bit more indirect to read?

steveukx · 2022-05-09T11:52:11Z

packages/modular-scripts/react-scripts/config/webpack.config.js

@@ -135,6 +157,35 @@ module.exports = function (webpackEnv) {
  };

  const webpackConfig = {
+    externals: isApp


As there are many app only configuration options here, might it be worth using a merge utility such as webpack-merge so the config can be:

const webpackConfig = merge( defaultConfig(), isApp && appConfig(), );

Agreed to do this in another PR.

packages/modular-scripts/src/addPackage.ts

packages/modular-scripts/src/esbuild-scripts/plugins/rewriteDependenciesPlugin.ts

packages/modular-scripts/src/esbuild-scripts/start/index.ts

packages/modular-scripts/src/serve.ts

Co-authored-by: Steve King <steve@mydev.co>

cangarugula · 2022-05-09T09:59:48Z

packages/modular-scripts/react-scripts/config/webpack.config.js

-const esbuildTargetFactory = process.env.ESBUILD_TARGET_FACTORY
-  ? JSON.parse(process.env.ESBUILD_TARGET_FACTORY)
-  : 'es2015';
+const isApp = process.env.MODULAR_IS_APP === 'true';


Suggestion: IS_MODULAR_APP? and also to bring it to lowercase before checking if it matches true

Suggestion: IS_MODULAR_APP?

It sounds better, but we have this half-convention of prepending env variables passed to the Webpack configuration with MODULAR_:

MODULAR_ROOT: modularRoot, MODULAR_PACKAGE: target, MODULAR_PACKAGE_NAME: targetName,

also to bring it to lowercase before checking if it matches true

It's passed only by our code, so we're pretty sure it's true (unless JSON.stringify(boolean) changes case), but if you think it's more hygienic I can do that.

cangarugula · 2022-05-09T10:04:37Z

packages/modular-scripts/react-scripts/config/webpack.config.js

+  : 'es2020';
+
+const { externalDependencies } = process.env.MODULAR_PACKAGE_DEPS
+  ? JSON.parse(process.env.MODULAR_PACKAGE_DEPS)


Won't this throw an error if MODULAR_PACKAGE_DEPS is anything other than an object or boolean? Can we be more defensive with this?

If we're only going to be using externalDependencies, can we just have the dependencies in the root of the object instead of nesting it?

Oh I see. This env var is generated at time of execution and passed in with no expectation of user configuration.

If that's the case, why doesn't the build script just fetch the external dependencies itself instead of reading it from the env var?

If that's the case, why doesn't the build script just fetch the external dependencies itself instead of reading it from the env var?

Because then we would need to either compile our getPackageDependencies.ts into javascript at runtime for webpack.config.js to use it, or "downgrade" it to js. There is no easy way for the webpack config to invoke our typescript utilities, and we use getPackageDependencies for other things (possibly even ghost testing in the future).

The real problem here is that webpack.config.js is not invoked by us, but by Webpack itself and we have to pass parameters to it via the environment, which is something we inherited from create-react-app.

I'm sure there's a way to solve this in a better way (ie generate a JSON configuration file from our build scripts), and I would like to do that, but that's for another PR.

Is this script run in the same process as the script that generates the configuration? If so we could switch to using process.modular = { packageDependencies, isApp, ... }

Is this script run in the same process as the script that generates the configuration? If so we could switch to using process.modular = { packageDependencies, isApp, ... }

I'm not really sure what you mean, but this is not really a script we execute, it's the actual Webpack configuration (that can be a javascript file), executed by Webpack itself. What we do is a bit convoluted:

In build/index.ts we call all our ts utilities, get the dependencies, set the environment variables and start a new javascript process in via execa - the process is contained in react-scripts/scripts/build.js

build.js starts webpack, with const compiler = webpack(configFactory('production'));. This is taken directly from CRA.

The webpack library starts a Webpack process, which in turn executes the configuration at webpack.config.js (this file, which needs to be js since is directly required by Webpack, afaik).

Agreed to do this in another PR.

cangarugula · 2022-05-09T14:24:16Z

packages/modular-scripts/react-scripts/config/webpack.config.js

+  const string = `
+  import ReactDOM from 'react-dom'
+  import React from 'react';
+  import Component from './src/index.tsx';


Should we template in paths.appIndexJs in case the component is being exported out of an index.jsx file?

good catch, done!

cangarugula · 2022-05-09T14:29:18Z

packages/modular-scripts/src/addPackage.ts

@@ -129,7 +130,7 @@ async function addPackage({

  const modularTemplateType = modularTemplatePackageJson?.modular
    ?.templateType as string;
-  if (!['app', 'view', 'package'].includes(modularTemplateType)) {
+  if (!['app', 'esm-view', 'view', 'package'].includes(modularTemplateType)) {
    throw new Error(
      `${templateName} has modular type: ${modularTemplateType}, which does not exist, please use update this template`,


To confirm, we're going to have to publish a modular-template-esm-view package as well.

Already done in advance, we've been already burned by this :) -> https://www.npmjs.com/package/modular-scripts

cangarugula · 2022-05-09T14:55:25Z

packages/modular-scripts/src/build/index.ts

+    );
+
+    // Create and write trampoline file
+    const trampolineBuildResult = await createViewTrampoline(


createViewTrampoline uses esbuild which means it's not build system agnostic. How does this play with a standalone build run with USE_MODULAR_WEBPACK=true

We build the trampoline with esbuild either way. The trampoline is not user code, we 100% control its template, so how it's built makes no difference to the user. We only build it to correctly replace its deps (react-dom and react) with the CDN template + allow / block list that the user has provided, via the esbuild plugin.

cangarugula · 2022-05-09T14:58:15Z

packages/modular-scripts/src/esbuild-scripts/api.ts

+<html>
+  <body>
+    <div id="root"></div>
+    <script type="module" src="static/js/_trampoline.js"></script>


We should use the %PUBLIC_URL% template to keep in line with the other index.html files we allow configuration for.

Why? Users are not supposed to provide their own index.html for an esm-view.

If they have "homepage" set in the package.json, we want to interpolate that into as the PUBLIC_URL when we build apps with webpack (https://github.com/jpmorganchase/modular/blob/main/packages/modular-scripts/react-scripts/config/webpack.config.js#L164-L167 ) . If the esm-view is meant to be able to be built as a standalone app, we should mirror this. It'll be important for building in CI if the assets are going to be nested or not stored in root.

It'll be important for building in CI if the assets are going to be nested or not stored in root.

The only assets that are allowed for a standalone esm-view are cssEntryPoint, jsEntryPoint and the runtime index.js if we're modular starting the application. We insert them dynamically - respecting the %PUBLIC_URL% - in the createIndex function just below.

If we set the PUBLIC_URL for the js and css entrypoints, why not also set them for the trampoline file?
<script type="module" src="%PUBLIC_URL%/static/js/_trampoline.js"></script>

If we set the PUBLIC_URL for the js and css entrypoints, why not also set them for the trampoline file? <script type="module" src="%PUBLIC_URL%/static/js/_trampoline.js"></script>

Ah, ok, I understand. This sparked a mini refactoring of api.ts, with common code in just one place and a better interface, plus obviously %PUBLIC_URL% interpolation. Done.

cangarugula · 2022-05-11T08:41:14Z

packages/modular-scripts/src/addPackage.ts

@@ -50,6 +50,7 @@ async function addPackage({
        choices: [
          { title: 'A plain package', value: 'package' },
          { title: 'A view within an application', value: 'view' },


Maybe to be clear what the difference between view and esm-view, could we update the title of view to indicate it's not an esm?

@cangarugula Happy to change it. Not sure of how to word this though, "A non-ESM view within an application" sounds bad. Do you have suggestions?

I'll bring some more options to the table to help us think through what we're trying to convey.

The first and most concise is to simply say what these things are and require the reader to view the docs to understand what "view" and "app" mean.

A plain package

A view compiled to ES Modules

A view compiled to a bundle file

A standalone app

Choose my own template

Following my previous logic, we could even shorten "A standalone app" to "An app".

However, if we wanted to be more descriptive, we could throw in an indication of how these things relate to micro-frontends, which is modular's thing:

A plain package

A micro-frontend view compiled to ES Modules

A micro-frontend view compiled to a bundle file

A micro-frontend container app

Choose my own template

We can even shorten things a little bit:

A plain package

A micro-frontend view (ES Modules)

A micro-frontend view (bundle file)

A micro-frontend container app

Choose my own template

What do people think?

Should we add the type to better disambiguate?

A plain package (package)

A micro-frontend view compiled to ES Modules (esm-view)

A micro-frontend view compiled to a bundle file (view)

A micro-frontend container app (app)

Choose my own template

@cangarugula @steveukx

Should we also add "react" in any of these titles or is that already obvious?

We should probably just choose one and change it in the future if we think it's confusing. I'll start a poll later.

@cangarugula @benpryke and @steveukx chose Option C. I incorporated it and:

Incorporated @benpryke's suggestion of not repeating the word "app"

Incorporated @cangarugula's preference of specifying "react" for esm-views, views and apps

Cristiano Belloni and others added 30 commits February 18, 2022 16:28

Bring webpack configuration to a valid (but not working) state

c5c50db

webpack build working

f139c67

Remove sourcemap obsolete flag

a2c91cb

Redo ForkTsCheckerWebpackPlugin conf

3d3352d

Fix stats type mismatch

5fcacfa

start.js

8fb8342

dev server working but trying to typecheck node_modules

b20e694

webpack start is working

bdfd0ec

Change css minimizer to remove deprecation

8986bd9

Remove diagnostics

9bd0894

Update snapshots

510fb6b

Support new version of minimize

92597a4

Add explaining comment

8bf2aea

Merge remote-tracking branch 'origin/main' into feature/experiment-we…

c6df324

…bpack-5

Update yarn.lock

5a340d0

Merge remote-tracking branch 'origin/main' into feature/experiment-we…

3122acf

…bpack-5

Merge remote-tracking branch 'origin/main' into feature/experiment-we…

1f7a830

…bpack-5

Merge remote-tracking branch 'origin/main' into feature/experiment-we…

8b1f89b

…bpack-5

Update snapshots

f075c72

Update all snapshots

7c98350

Update snapshot hash

5f3c001

Add missing quotes

7b45909

Merge branch 'main' into feature/experiment-webpack-5

f97c6f2

Remove unwanted media dist files

bc9cb4c

remove tsbuildinfo

87964dd

Merge remote-tracking branch 'origin/main' into feature/experiment-we…

187f866

…bpack-5

Make test names decoupled from hashes

e1abbef

Update test snapshots

8816b88

Create silver-dolphins-remember.md

e5d772f

Enter pre-release mode

51e770e

cristiano-belloni marked this pull request as ready for review May 5, 2022 10:48

Cristiano Belloni and others added 2 commits May 5, 2022 15:00

Merge remote-tracking branch 'origin/main' into feature/esm-views

455cb57

Create proud-starfishes-stare.md

c051e92

cristiano-belloni requested review from LukeSheard, benpryke, cangarugula and steveukx May 5, 2022 16:53

steveukx reviewed May 9, 2022

View reviewed changes

cristiano-belloni and others added 5 commits May 9, 2022 15:14

Update packages/modular-scripts/src/addPackage.ts

94e3790

Co-authored-by: Steve King <steve@mydev.co>

Update packages/modular-scripts/src/serve.ts

986aea8

Co-authored-by: Steve King <steve@mydev.co>

Remove assertion

6260a07

Use Object.create

b555499

Use map for importMap

c53c5fa

cangarugula reviewed May 9, 2022

View reviewed changes

Cristiano Belloni added 2 commits May 9, 2022 17:47

Use paths to get the entrypoint file

1444b6f

Simplify index creation

dd64eab

cristiano-belloni requested review from steveukx and cangarugula May 10, 2022 14:05

steveukx added this to the ESM View Support milestone May 10, 2022

cangarugula reviewed May 11, 2022

View reviewed changes

cristiano-belloni requested a review from cangarugula May 11, 2022 18:06

Wording of add options

925b5a1

cangarugula approved these changes May 12, 2022

View reviewed changes

cristiano-belloni merged commit da4eeed into main May 12, 2022

cristiano-belloni deleted the feature/esm-views branch May 12, 2022 13:35

This was referenced May 12, 2022

Version Packages #1715

Closed

Version Packages benpryke/modular#1

Open

Version Packages #1733

Closed

github-actions bot mentioned this pull request May 24, 2022

Version Packages #1761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/esm views #1686

Feature/esm views #1686

cristiano-belloni commented May 3, 2022 •

edited

Loading

coveralls commented May 5, 2022 •

edited

Loading

steveukx May 9, 2022

cristiano-belloni May 9, 2022

steveukx May 9, 2022

cristiano-belloni May 10, 2022

cangarugula May 9, 2022

cristiano-belloni May 9, 2022

cangarugula May 9, 2022

cangarugula May 9, 2022

cangarugula May 9, 2022

cristiano-belloni May 9, 2022 •

edited

Loading

steveukx May 10, 2022

cristiano-belloni May 10, 2022

cristiano-belloni May 10, 2022

cangarugula May 9, 2022

cristiano-belloni May 9, 2022

cangarugula May 9, 2022

cristiano-belloni May 9, 2022

cangarugula May 9, 2022

cristiano-belloni May 9, 2022 •

edited

Loading

cangarugula May 9, 2022

cristiano-belloni May 9, 2022

cangarugula May 9, 2022

cristiano-belloni May 9, 2022 •

edited

Loading

cangarugula May 10, 2022

cristiano-belloni May 10, 2022

cangarugula May 11, 2022

cristiano-belloni May 11, 2022

benpryke May 11, 2022

cristiano-belloni May 11, 2022

cangarugula May 12, 2022

cristiano-belloni May 12, 2022

cristiano-belloni May 12, 2022 •

edited

Loading

Feature/esm views #1686

Feature/esm views #1686

Conversation

cristiano-belloni commented May 3, 2022 • edited Loading

coveralls commented May 5, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristiano-belloni May 9, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristiano-belloni May 9, 2022 • edited Loading

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

cristiano-belloni May 9, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristiano-belloni May 12, 2022 • edited Loading

Choose a reason for hiding this comment

cristiano-belloni commented May 3, 2022 •

edited

Loading

coveralls commented May 5, 2022 •

edited

Loading

cristiano-belloni May 9, 2022 •

edited

Loading

cristiano-belloni May 9, 2022 •

edited

Loading

cristiano-belloni May 9, 2022 •

edited

Loading

cristiano-belloni May 12, 2022 •

edited

Loading