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

Feature/esm views #1686

Merged
merged 89 commits into from
May 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
89 commits
Select commit Hold shift + click to select a range
c5c50db
Bring webpack configuration to a valid (but not working) state
Feb 18, 2022
f139c67
webpack build working
Feb 21, 2022
a2c91cb
Remove sourcemap obsolete flag
Feb 21, 2022
3d3352d
Redo ForkTsCheckerWebpackPlugin conf
Feb 21, 2022
5fcacfa
Fix stats type mismatch
Feb 21, 2022
8fb8342
start.js
Feb 22, 2022
b20e694
dev server working but trying to typecheck node_modules
Feb 23, 2022
bdfd0ec
webpack start is working
Feb 23, 2022
8986bd9
Change css minimizer to remove deprecation
Feb 23, 2022
9bd0894
Remove diagnostics
Feb 23, 2022
510fb6b
Update snapshots
Feb 24, 2022
92597a4
Support new version of minimize
Feb 24, 2022
8bf2aea
Add explaining comment
Feb 24, 2022
c6df324
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Feb 28, 2022
5a340d0
Update yarn.lock
Feb 28, 2022
3122acf
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Mar 4, 2022
1f7a830
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Mar 7, 2022
8b1f89b
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Mar 8, 2022
f075c72
Update snapshots
Mar 8, 2022
7c98350
Update all snapshots
Mar 8, 2022
5f3c001
Update snapshot hash
Mar 8, 2022
7b45909
Add missing quotes
Mar 8, 2022
f97c6f2
Merge branch 'main' into feature/experiment-webpack-5
cristiano-belloni Mar 11, 2022
bc9cb4c
Remove unwanted media dist files
Mar 11, 2022
87964dd
remove tsbuildinfo
Mar 11, 2022
187f866
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Mar 11, 2022
e1abbef
Make test names decoupled from hashes
Mar 14, 2022
8816b88
Update test snapshots
Mar 14, 2022
e5d772f
Create silver-dolphins-remember.md
cristiano-belloni Mar 16, 2022
51e770e
Enter pre-release mode
Mar 16, 2022
b16fae1
Merge remote-tracking branch 'origin/main' into release/webpack-5
Mar 17, 2022
681ab74
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Mar 17, 2022
07052ac
Remove requireEnsure
Mar 21, 2022
55df9e0
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Mar 22, 2022
76ce1bb
Merge remote-tracking branch 'origin/main' into release/webpack-5
Mar 25, 2022
dc846d8
Regenerate pre.json
Mar 25, 2022
43c42b7
remove workflow rule
Mar 25, 2022
bd5e86b
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Mar 25, 2022
50332fd
Merge remote-tracking branch 'origin/main' into release/webpack-5
Mar 28, 2022
fb7db88
Update pre.json
Mar 28, 2022
5a6dfdc
Merge remote-tracking branch 'origin/main' into feature/experiment-we…
Mar 28, 2022
47e1ec4
Update snapshots
Mar 28, 2022
64f46f7
Update build snapshots
Mar 28, 2022
41ded67
Update browser versions
Mar 28, 2022
b2ca58b
Merge pull request #1421 from jpmorganchase/feature/experiment-webpack-5
cristiano-belloni Mar 29, 2022
8cfe64f
Version Packages (alpha-webpack5)
github-actions[bot] Mar 29, 2022
d8562e6
Merge pull request #1521 from jpmorganchase/changeset-release/release…
cristiano-belloni Mar 29, 2022
4f1af82
Revert "Revert engine range (#1518)"
Apr 5, 2022
d2aacf6
Revert "fix node-12 incompatible javascript (#1517)"
Apr 5, 2022
d2b10e9
Remove node 12 support
Apr 5, 2022
69da976
Add changeset
Apr 5, 2022
c40158b
Add node 18 in CI
Apr 5, 2022
074bff4
Revert "Add node 18 in CI"
Apr 5, 2022
4baca0a
Hardcode node to v14
Apr 6, 2022
9f48679
support node 16 explicitly and run test workflow against target node …
LukeSheard Apr 6, 2022
8115519
fix node versions so that tests run
LukeSheard Apr 6, 2022
8b3f1f5
Create dirty-mugs-double.md
LukeSheard Apr 6, 2022
8901b9f
fix node versions so that tests run and remove 15 since it's unsupported
LukeSheard Apr 6, 2022
0a0bc65
Merge remote-tracking branch 'origin/bugfix/workflow-node-version' in…
Apr 7, 2022
4abf766
Raise version to be compatible with eslint
Apr 7, 2022
4f9cba9
Version Packages (alpha-webpack5)
github-actions[bot] Apr 7, 2022
a31fb3f
Merge pull request #1554 from jpmorganchase/changeset-release/release…
cristiano-belloni Apr 8, 2022
af674c5
Support for esm-view type in react scripts
Apr 26, 2022
d6d2e8c
Add esm-view into modular types
Apr 26, 2022
49c0604
Better names for isView + activate esm-views in start
Apr 26, 2022
69c480c
Update tests
Apr 26, 2022
93e0adb
Add tests and way of adding esm-view
Apr 26, 2022
a60bec5
Better comments/names
Apr 26, 2022
b0ccaf4
Merge remote-tracking branch 'origin/main' into feature/esm-views
May 3, 2022
8c0a688
Add esm-view type
May 4, 2022
375c7e8
Add package modular-template-esm-view
May 4, 2022
309cf20
Fix logger error
May 4, 2022
4e46075
fix esm view tests
May 4, 2022
865b80f
Fix app tests
May 4, 2022
f168606
Fix view tests
May 4, 2022
2130f56
Fix build tests
May 4, 2022
7fa41ae
Remove residual changelogs
May 4, 2022
d34acec
Fix app node env tests
May 4, 2022
c189293
Fix WorkspaceInfo test
May 5, 2022
455cb57
Merge remote-tracking branch 'origin/main' into feature/esm-views
May 5, 2022
c051e92
Create proud-starfishes-stare.md
cristiano-belloni May 5, 2022
94e3790
Update packages/modular-scripts/src/addPackage.ts
cristiano-belloni May 9, 2022
986aea8
Update packages/modular-scripts/src/serve.ts
cristiano-belloni May 9, 2022
6260a07
Remove assertion
May 9, 2022
b555499
Use Object.create
May 9, 2022
c53c5fa
Use map for importMap
May 9, 2022
1444b6f
Use paths to get the entrypoint file
May 9, 2022
dd64eab
Simplify index creation
May 10, 2022
925b5a1
Wording of add options
May 12, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/proud-starfishes-stare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"modular-scripts": minor
---

Add `esm-view` modular type
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ node_modules
packages/sample-app
packages/sample-esbuild-app
packages/sample-view
packages/sample-esm-view
packages/sample-package
packages/nested
packages/sample-library-package
Expand Down
9 changes: 7 additions & 2 deletions babel.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ module.exports = (api) => {
},
},
],
'@babel/preset-typescript',
'@babel/preset-react',
],
overrides: [
{
test: /\.tsx?$/,
presets: ['@babel/preset-typescript'],
},
],
plugins: ['@babel/plugin-proposal-class-properties'],
};
};
205 changes: 167 additions & 38 deletions packages/modular-scripts/react-scripts/config/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,20 @@ const ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin');
const postcssNormalize = require('postcss-normalize');
const isCI = require('is-ci');

const esbuildTargetFactory = process.env.ESBUILD_TARGET_FACTORY
? JSON.parse(process.env.ESBUILD_TARGET_FACTORY)
: 'es2015';
const isApp = process.env.MODULAR_IS_APP === 'true';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const isEsmView = !isApp;
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


// If it's an app, set it at ESBUILD_TARGET_FACTORY or default to es2015
// If it's not an app it's an ESM view, then we need es2020
const esbuildTargetFactory = isApp
? process.env.ESBUILD_TARGET_FACTORY
? JSON.parse(process.env.ESBUILD_TARGET_FACTORY)
: 'es2015'
: 'es2020';

const { externalDependencies } = process.env.MODULAR_PACKAGE_DEPS
? JSON.parse(process.env.MODULAR_PACKAGE_DEPS)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@cristiano-belloni cristiano-belloni May 9, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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, ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. 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
  2. build.js starts webpack, with const compiler = webpack(configFactory('production'));. This is taken directly from CRA.
  3. 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed to do this in another PR.

: {};

// Source maps are resource heavy and can cause out of memory issue for large source files.
const shouldUseSourceMap = process.env.GENERATE_SOURCEMAP !== 'false';
Expand Down Expand Up @@ -60,6 +71,17 @@ const sassModuleRegex = /\.module\.(scss|sass)$/;
module.exports = function (webpackEnv) {
const isEnvDevelopment = webpackEnv === 'development';
const isEnvProduction = webpackEnv === 'production';
const isEsmViewDevelopment = isEsmView & isEnvDevelopment;

// This is needed if we're serving a ESM view in development node, since it won't be defined in the view dependencies.
if (externalDependencies.react && isEsmViewDevelopment) {
externalDependencies['react-dom'] = externalDependencies.react;
}

// Create a map of external dependencies if we're building a ESM view
const dependencyMap = isEsmView
? createExternalDependenciesMap(externalDependencies)
: {};

// Variable used for enabling profiling in Production
// passed into alias object. Uses a flag if passed into the build command
Expand Down Expand Up @@ -135,6 +157,35 @@ module.exports = function (webpackEnv) {
};

const webpackConfig = {
externals: isApp
Copy link
Contributor

Choose a reason for hiding this comment

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

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(),
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed to do this in another PR.

? undefined
: function ({ request }, callback) {
const parsedModule = parsePackageName(request);

// If the module is absolute and it is in the import map, we want to externalise it
if (
parsedModule &&
parsedModule.dependencyName &&
dependencyMap[parsedModule.dependencyName]
) {
const { dependencyName, submodule } = parsedModule;

const toRewrite = `${dependencyMap[dependencyName]}${
submodule ? `/${submodule}` : ''
}`;

return callback(null, toRewrite);
}
// Otherwise we just want to bundle it
return callback();
},

externalsType: isApp ? undefined : 'module',
experiments: {
outputModule: isApp ? undefined : true,
},
// Workaround for this bug: https://stackoverflow.com/questions/53905253/cant-set-up-the-hmr-stuck-with-waiting-for-update-signal-from-wds-in-cons
target: 'web',
mode: isEnvProduction ? 'production' : isEnvDevelopment && 'development',
// Stop compilation early in production
bail: isEnvProduction,
Expand All @@ -145,8 +196,15 @@ module.exports = function (webpackEnv) {
: isEnvDevelopment && 'cheap-module-source-map',
// These are the "entry points" to our application.
// This means they will be the "root" imports that are included in JS bundle.
entry: paths.appIndexJs,
// We bundle a virtual file to trampoline the ESM view as an entry point if we're starting it (ESM views have no ReactDOM.render)
entry: isEsmViewDevelopment ? getVirtualTrampoline() : paths.appIndexJs,
output: {
module: isApp ? undefined : true,
library: isApp
? undefined
: {
type: 'module',
},
// The build folder.
path: isEnvProduction ? paths.appBuild : undefined,
// Add /* filename */ comments to generated require()s in the output.
Expand Down Expand Up @@ -223,15 +281,19 @@ module.exports = function (webpackEnv) {
// Automatically split vendor and commons
// https://twitter.com/wSokra/status/969633336732905474
// https://medium.com/webpack/webpack-4-code-splitting-chunk-graph-and-the-splitchunks-optimization-be739a861366
splitChunks: {
chunks: 'all',
},
splitChunks: isApp
? {
chunks: 'all',
}
: undefined,
// Keep the runtime chunk separated to enable long term caching
// https://twitter.com/wSokra/status/969679223278505985
// https://github.com/facebook/create-react-app/issues/5358
runtimeChunk: {
name: (entrypoint) => `runtime-${entrypoint.name}`,
},
runtimeChunk: isApp
? {
name: (entrypoint) => `runtime-${entrypoint.name}`,
}
: undefined,
},
resolve: {
// This allows you to set a fallback for where webpack should look for modules.
Expand All @@ -249,7 +311,7 @@ module.exports = function (webpackEnv) {
// for React Native Web.
extensions: paths.moduleFileExtensions
.map((ext) => `.${ext}`)
.filter((ext) => useTypeScript || !ext.includes('ts')),
.filter((ext) => useTypeScript || !isApp || !ext.includes('ts')),
alias: {
// Support React Native Web
// https://www.smashingmagazine.com/2016/08/a-glimpse-into-the-future-with-react-native-for-web/
Expand Down Expand Up @@ -458,43 +520,64 @@ module.exports = function (webpackEnv) {
},
plugins: [
// Generates an `index.html` file with the <script> injected.
new HtmlWebpackPlugin(
Object.assign(
{},
{
inject: true,
template: paths.appHtml,
},
isEnvProduction
? {
minify: {
removeComments: true,
collapseWhitespace: true,
removeRedundantAttributes: true,
useShortDoctype: true,
removeEmptyAttributes: true,
removeStyleLinkTypeAttributes: true,
keepClosingSlash: true,
minifyJS: true,
minifyCSS: true,
minifyURLs: true,
},
}
: undefined,
),
),
isApp
? new HtmlWebpackPlugin(
Object.assign(
{},
{
inject: true,
template: paths.appHtml,
},
isEnvProduction
? {
minify: {
removeComments: true,
collapseWhitespace: true,
removeRedundantAttributes: true,
useShortDoctype: true,
removeEmptyAttributes: true,
removeStyleLinkTypeAttributes: true,
keepClosingSlash: true,
minifyJS: true,
minifyCSS: true,
minifyURLs: true,
},
}
: undefined,
),
)
: isEsmViewDevelopment
? // We need to provide a synthetic index.html in case we're starting a ESM view
new HtmlWebpackPlugin(
Object.assign(
{},
{
inject: true,
templateContent: `
<html>
<body>
<div id="root"></div>
</body>
</html>
`,
scriptLoading: 'module',
},
),
)
: false,
// Inlines the webpack runtime script. This script is too small to warrant
// a network request.
// https://github.com/facebook/create-react-app/issues/5358
isEnvProduction &&
isApp &&
isEnvProduction &&
shouldInlineRuntimeChunk &&
new InlineChunkHtmlPlugin(HtmlWebpackPlugin, [/runtime-.+[.]js/]),
// Makes some environment variables available in index.html.
// The public URL is available as %PUBLIC_URL% in index.html, e.g.:
// <link rel="icon" href="%PUBLIC_URL%/favicon.ico">
// It will be an empty string unless you specify "homepage"
// in `package.json`, in which case it will be the pathname of that URL.
new InterpolateHtmlPlugin(HtmlWebpackPlugin, env.raw),
isApp && new InterpolateHtmlPlugin(HtmlWebpackPlugin, env.raw),
// This gives some necessary context to module not found errors, such as
// the requesting resource.
new ModuleNotFoundPlugin(paths.appPath),
Expand Down Expand Up @@ -651,3 +734,49 @@ module.exports = function (webpackEnv) {

return webpackConfig;
};

function createExternalDependenciesMap(externalDependencies) {
const externalCdnTemplate =
process.env.EXTERNAL_CDN_TEMPLATE ||
'https://cdn.skypack.dev/[name]@[version]';

return Object.entries(externalDependencies).reduce(
(acc, [name, version]) => ({
...acc,
[name]: externalCdnTemplate
.replace('[name]', name)
.replace('[version]', version),
}),
{},
);
}

const packageRegex =
/^(@[a-z0-9-~][a-z0-9-._~]*)?\/?([a-z0-9-~][a-z0-9-._~]*)\/?(.*)/;
function parsePackageName(name) {
const parsedName = packageRegex.exec(name);
if (!parsedName) {
return;
}
const [_, scope, module, submodule] = parsedName;
const dependencyName = (scope ? `${scope}/` : '') + module;
return { dependencyName, scope, module, submodule };
}

// Virtual entrypoint if we're starting a ESM view - see https://github.com/webpack/webpack/issues/6437
function getVirtualTrampoline() {
const entryPointPath = `'./${path.relative(
paths.appPath,
paths.appIndexJs,
)}'`;
const string = `
import ReactDOM from 'react-dom'
import React from 'react';
import Component from ${entryPointPath};
const DOMRoot = document.getElementById('root');
ReactDOM.render(React.createElement(Component, null), DOMRoot);
`;

const base64 = Buffer.from(string).toString('base64');
return `./src/_trampoline.js!=!data:text/javascript;base64,${base64}`;
}
5 changes: 5 additions & 0 deletions packages/modular-scripts/src/__tests__/TestEsmView.test-tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import * as React from 'react';

export default function SampleESMView(): JSX.Element {
return <div data-testid="test-this">this is a modular esm-view</div>
}
12 changes: 12 additions & 0 deletions packages/modular-scripts/src/__tests__/TestViewPackages.test-tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as React from 'react';
import get from 'lodash/get';
import merge from 'lodash.merge';
import { difference } from 'lodash';

export default function SampleView(): JSX.Element {
return (
<div>
<pre>{JSON.stringify({ get, merge, difference })}</pre>
</div>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,20 @@ exports[`when working with a NODE_ENV app WHEN building with esbuild can generat
"
`;

exports[`when working with a NODE_ENV app WHEN building with webpack can generate a js/main.1c6de6d0.js 1`] = `
exports[`when working with a NODE_ENV app WHEN building with webpack can generate a js/main.a482480b.js 1`] = `
"\\"use strict\\";
(globalThis.webpackChunknode_env_app =
globalThis.webpackChunknode_env_app || []).push([
(self.webpackChunknode_env_app = self.webpackChunknode_env_app || []).push([
[179],
{
908: () => {
console.log(\\"production\\");
},
},
(o) => {
var e;
(e = 908), o((o.s = e));
(e) => {
var n;
(n = 908), e((e.s = n));
},
]);
//# sourceMappingURL=main.1c6de6d0.js.map
//# sourceMappingURL=main.a482480b.js.map
"
`;
Loading