refactor: bundling external fallbacks with webpack #637

giulianok · 2024-04-09T23:07:19Z

Provide a general summary of your changes in the Title above.

Bundling external fallbacks with webpack instead of esbuild

Description

Describe your changes in detail

External Fallbacks are currently being bundled with esbuild. There are certain externals that use libs such as crypto that require a fallback to a polyfill (e.g. crypto-browserify) in order to properly work on the browser. esbuild supports it via aliases but only for the code that's being transpiled and not for its dependencies. webpack solves by via the fallback flag which tries to resolve the dependency and defaults to a specified one in case it cannot, even for dependencies.

We could have created an esbuild plugin to implement the fallback feature from webpack, but we decided to instead use webpack to bundle it to be consistent with production and because we are unsure when esbuild will be used for production bundles.

This project only accepts pull requests related to open issues.

If suggesting a new feature or change, please discuss it in an issue first.

Motivation

Why is this change required? What issue does it resolve?

Test Conditions

Describe in detail how the changes are tested.

Include details of your testing environment, and the tests you ran to.

How does your change affect the rest of the code.

Types of changes

Check boxes that apply:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (adding or updating documentation)
Dependency update
Security update

Checklist

Check boxes that apply:

My code follows the code style of this project.
My change requires a change to the documentation and I have updated the documentation accordingly.
These changes should be applied to a maintenance branch.
I have added the Apache 2.0 license header to any new files created.

packages/one-app-bundler/bin/bundle-external-fallbacks.js

packages/one-app-bundler/utils/getExternalLibraryName.js

.eslintignore

jest.cjs.config.js

packages/one-app-bundler/__tests__/webpack/plugins/register-external-injector.spec.js

10xLaCroixDrinker · 2024-04-13T15:30:50Z

packages/one-app-bundler/bin/webpackCallback.js

@@ -15,9 +15,11 @@
 import chalk from 'chalk';
 import fs from 'node:fs';
 import path from 'node:path';
+import snakeCase from 'lodash.snakecase';


Can we just implement this here instead of adding a dependency (esp lodash)

function toSnakeCase(str) { return str .replace(/[\s-]+/g, '_') // Replace spaces and dashes with _ .replace(/([A-Z])/g, '_$1') // Prefix uppercase letters with _ .toLowerCase() // Convert the whole string to lowercase .replace(/^_/, ''); // Remove leading underscore if present }

why can't we just go with lodash? we used it before in the dev bundler and does not affect the output/bundle

We already have all of lodash we don't need lodash.snakecase specifically as well right?

packages/one-app-bundler/webpack/externalFallbacks/webpack.client.js

10xLaCroixDrinker · 2024-04-13T15:34:36Z

packages/one-app-bundler/webpack/externalFallbacks/webpack.client.js

+      resolve: {
+        mainFields: MAIN_FIELDS,
+        modules: [packageRoot, 'node_modules'],
+        extensions: ['.js', '.jsx', '.ts', '.tsx'],


Suggested change

extensions: ['.js', '.jsx', '.ts', '.tsx'],

extensions: ['.js', '.jsx', '.ts', '.tsx', '.cjs'],

i don't think this is needed since it's strange to have client side code using commonjs

@giulianok I had to fix this last week with the latest redux release, so it is needed
#633

10xLaCroixDrinker · 2024-04-13T15:35:30Z

packages/one-app-bundler/webpack/externalFallbacks/webpack.client.js

+      module: {
+        rules: [
+          {
+            test: /[jt]sx?$/,


Suggested change

test: /[jt]sx?$/,

test: /\.c?[jt]sx?$/,

i don't think this is needed since it's strange to have client side code using commonjs

https://github.com/americanexpress/one-app-cli/pull/637/files#r1576215080

packages/one-app-bundler/webpack/externalFallbacks/webpack.client.js

packages/one-app-bundler/webpack/externalFallbacks/webpack.server.js

Co-authored-by: Matthew Mallimo <matthew.c.mallimo@aexp.com>

Co-authored-by: Jamie King <jamie.king@aexp.com>

…ps://github.com/americanexpress/one-app-cli into refactor/external-fallbacks-bundle-with-webpack

Matthew-Mallimo · 2024-04-15T21:02:44Z

packages/one-app-bundler/utils/writeToModuleConfig.js

+  fs.writeFileSync(configPath, JSON.stringify({
+    ...config,
+    ...newData,
+  }, null, 2));


Can we save some bytes by not pretty printing the json?

i don't think it will make a huge diff but why not

packages/one-app-bundler/webpack/plugins/register-external-injector.js

dogpatch626 · 2024-04-16T14:34:39Z

packages/one-app-bundler/webpack/externalFallbacks/webpack.client.js

+          util: resolve('util'),
+          vm: resolve('vm-browserify'),
+          zlib: resolve('browserify-zlib'),
+        },


If we are pollyfilling this ^ all of these modules in fallback: elsewhere, does it make sense to do it again here within the externalFallbacks?

https://github.com/americanexpress/one-app-cli/blob/main/packages/one-app-bundler/webpack/module/webpack.client.js These are pollyfills that were provided by webpack 4 and no longer are so we manually pollyfill them here. probably should not provide it again? maybe my understanding of what actually is happening in the externalFallbacks version is off

not sure if I follow.

we aren't actually pollyfilling, we provide a fallback, which only uses the pollyfil if the actual lib cannot be used / isn't available. We do it for the modules and we need to do it for external fallbacks since they are separate bundles

mm okay I think im mainly just trying to figure out if this work is being duplicated. Because in module/webpack.client.js we pollyfill, and here we fallback the pollyfill it seems so if the pollyfill is found it should not duplicate such work

@giulianok I think Danny's point is that this code is repeated. Maybe we can extract this out

refactor: bundling external fallbacks with webpack

ec30cd3

github-actions bot added one-app-bundler one-app-team-review-requested labels Apr 9, 2024

giulianok added 2 commits April 10, 2024 14:22

fix: unit tests

080cdaa

chore: merge conflicts

2bd49b6

giulianok marked this pull request as ready for review April 10, 2024 18:49

giulianok requested review from a team as code owners April 10, 2024 18:49

Matthew-Mallimo reviewed Apr 10, 2024

View reviewed changes

packages/one-app-bundler/bin/bundle-external-fallbacks.js Outdated Show resolved Hide resolved

Matthew-Mallimo reviewed Apr 10, 2024

View reviewed changes

packages/one-app-bundler/utils/getExternalLibraryName.js Outdated Show resolved Hide resolved

bishnubista previously approved these changes Apr 11, 2024

View reviewed changes

10xLaCroixDrinker suggested changes Apr 13, 2024

View reviewed changes

chore: update reduce to match standard naming

d2060d7

Co-authored-by: Matthew Mallimo <matthew.c.mallimo@aexp.com>

giulianok dismissed bishnubista’s stale review via d2060d7 April 15, 2024 15:54

giulianok and others added 4 commits April 15, 2024 11:56

refactor: use node prefix when importing

fd6b556

Co-authored-by: Jamie King <jamie.king@aexp.com>

chore: handled feedback from reviews

2e7b864

Merge branch 'refactor/external-fallbacks-bundle-with-webpack' of htt…

6b3f21b

…ps://github.com/americanexpress/one-app-cli into refactor/external-fallbacks-bundle-with-webpack

chore: feedback

12e8ebd

Matthew-Mallimo reviewed Apr 15, 2024

View reviewed changes

packages/one-app-bundler/webpack/plugins/register-external-injector.js Show resolved Hide resolved

dogpatch626 previously approved these changes Apr 16, 2024

View reviewed changes

chore: review feedback

08d7262

giulianok dismissed dogpatch626’s stale review via 08d7262 April 16, 2024 18:33

10xLaCroixDrinker closed this May 15, 2024

10xLaCroixDrinker deleted the refactor/external-fallbacks-bundle-with-webpack branch May 15, 2024 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: bundling external fallbacks with webpack #637

refactor: bundling external fallbacks with webpack #637

giulianok commented Apr 9, 2024 •

edited

10xLaCroixDrinker Apr 13, 2024

giulianok Apr 16, 2024

code-forger Apr 17, 2024

10xLaCroixDrinker Apr 13, 2024

giulianok Apr 16, 2024

Matthew-Mallimo Apr 23, 2024

10xLaCroixDrinker Apr 13, 2024

giulianok Apr 16, 2024

Matthew-Mallimo Apr 23, 2024

Matthew-Mallimo Apr 15, 2024

giulianok Apr 16, 2024

dogpatch626 Apr 16, 2024

dogpatch626 Apr 16, 2024

giulianok Apr 16, 2024

dogpatch626 Apr 17, 2024

Matthew-Mallimo Apr 23, 2024

	extensions: ['.js', '.jsx', '.ts', '.tsx'],
	extensions: ['.js', '.jsx', '.ts', '.tsx', '.cjs'],

refactor: bundling external fallbacks with webpack #637

refactor: bundling external fallbacks with webpack #637

Conversation

giulianok commented Apr 9, 2024 • edited

Provide a general summary of your changes in the Title above.

Description

Describe your changes in detail

This project only accepts pull requests related to open issues.

If suggesting a new feature or change, please discuss it in an issue first.

Motivation

Why is this change required? What issue does it resolve?

Test Conditions

Describe in detail how the changes are tested.

Include details of your testing environment, and the tests you ran to.

How does your change affect the rest of the code.

Types of changes

Check boxes that apply:

Checklist

Check boxes that apply:

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

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

giulianok commented Apr 9, 2024 •

edited