3.7.0 broke build with Rollup.

keean commented 2 years ago

What happened and what did you expect to happen?

I have been using Rollup to build a progressive web app using chime since version 1 of the SDK. Everything was working fine in 3.6.0, but 3.7.0 will no longer build.

Have you reviewed our existing documentation?

[X] GitHub issues
[X] FAQs page
[X] README page
[X] Feature-specific guides

Reproduction steps

3.6.0 would build without needing @rollup/plugin-json but this is needed to get a sensible error message out of rollup with 3.7.0:

Creating a browser bundle that depends on "os", "path", "url", "buffer", "http", "https", "stream", "process" and "util". You might need to include https://github.com/FredKSchott/rollup-plugin-polyfill-node
multi-entry.js:
amazon-chime-sdk-js - 1.59 MB (26.28%)
@fortawesome/free-solid-svg-icons - 1.01 MB (16.62%)
pdfjs-dist - 798.7 KB (12.90%)
app - 525.72 KB (8.49%)
@aws-sdk/client-chime-sdk-messaging - 525.7 KB (8.49%)
pako - 425.18 KB (6.87%)
web-streams-polyfill - 196.09 KB (3.17%)
@fortawesome/fontawesome-svg-core - 112.5 KB (1.82%)
@aws-sdk/client-sts - 105.91 KB (1.71%)
@aws-sdk/client-sso - 58.64 KB (0.95%)
utif - 56.91 KB (0.92%)
jpeg-lossless-decoder-js - 52.77 KB (0.85%)
protobufjs - 51.24 KB (0.83%)
ua-parser-js - 43.24 KB (0.70%)
pepjs - 43.23 KB (0.70%)
fast-xml-parser - 43.16 KB (0.70%)
entities - 41.84 KB (0.68%)
resize-observer-polyfill - 32.87 KB (0.53%)
@aws-crypto/sha256-js - 31.73 KB (0.51%)
dicom-parser - 31.23 KB (0.50%)
@aws-sdk/signature-v4 - 31.02 KB (0.50%)
dommatrix - 27.72 KB (0.45%)
@aws-sdk/smithy-client - 26.05 KB (0.42%)
rollup helpers - 24.79 KB (0.40%)
@aws-sdk/middleware-retry - 22.12 KB (0.36%)
tslib - 22.03 KB (0.36%)
@aws-sdk/credential-provider-imds - 18.85 KB (0.30%)
uuid - 15.27 KB (0.25%)
awesomplete - 12.76 KB (0.21%)
@protobufjs/float - 11.18 KB (0.18%)
@aws-sdk/config-resolver - 10.15 KB (0.16%)
@aws-crypto/util - 9.73 KB (0.16%)
@aws-sdk/node-http-handler - 9.21 KB (0.15%)
@aws-sdk/middleware-stack - 9.07 KB (0.15%)
@aws-sdk/credential-provider-ini - 8.72 KB (0.14%)
@aws-sdk/middleware-signing - 8.18 KB (0.13%)
detect-browser - 7.64 KB (0.12%)
@aws-sdk/credential-provider-sso - 6.82 KB (0.11%)
@aws-sdk/shared-ini-file-loader - 6.66 KB (0.11%)
@aws-sdk/property-provider - 5.95 KB (0.10%)
@aws-sdk/util-defaults-mode-node - 5.25 KB (0.08%)
@aws-sdk/middleware-user-agent - 3.9 KB (0.06%)
@aws-sdk/node-config-provider - 3.88 KB (0.06%)
@protobufjs/base64 - 3.85 KB (0.06%)
@aws-sdk/credential-provider-process - 3.56 KB (0.06%)
@protobufjs/utf8 - 3.29 KB (0.05%)
autosize - 3.17 KB (0.05%)
@aws-sdk/middleware-serde - 2.68 KB (0.04%)
@aws-sdk/credential-provider-web-identity - 2.58 KB (0.04%)
@aws-sdk/util-utf8-browser - 2.31 KB (0.04%)
@aws-sdk/credential-provider-node - 2.24 KB (0.04%)
@aws-sdk/util-user-agent-node - 2.19 KB (0.04%)
@aws-sdk/protocol-http - 2.13 KB (0.03%)
@protobufjs/eventemitter - 2.04 KB (0.03%)
@aws-sdk/middleware-logger - 1.97 KB (0.03%)
@aws-sdk/middleware-recursion-detection - 1.74 KB (0.03%)
@aws-sdk/middleware-content-length - 1.74 KB (0.03%)
@aws-sdk/service-error-classification - 1.69 KB (0.03%)
@protobufjs/aspromise - 1.55 KB (0.03%)
@aws-sdk/middleware-host-header - 1.47 KB (0.02%)
@protobufjs/pool - 1.21 KB (0.02%)
@aws-sdk/querystring-parser - 1.18 KB (0.02%)
@aws-sdk/credential-provider-env - 1.08 KB (0.02%)
@aws-sdk/querystring-builder - 1.07 KB (0.02%)
canvas - 1.04 KB (0.02%)
@aws-sdk/util-hex-encoding - 1.01 KB (0.02%)
@aws-sdk/hash-node - 986 B (0.02%)
@aws-sdk/util-buffer-from - 803 B (0.01%)
@aws-sdk/util-body-length-node - 676 B (0.01%)
@aws-sdk/util-base64-node - 627 B (0.01%)
@aws-sdk/url-parser - 564 B (0.01%)
@protobufjs/inquire - 544 B (0.01%)
@aws-sdk/util-config-provider - 524 B (0.01%)
@aws-sdk/util-utf8-node - 397 B (0.01%)
@aws-sdk/middleware-sdk-sts - 302 B (0.00%)
@aws-sdk/util-uri-escape - 207 B (0.00%)
@aws-sdk/util-middleware - 206 B (0.00%)
@aws-sdk/is-array-buffer - 199 B (0.00%)

It looks for some reason that 3.7.0 is pulling in a load of the aws-sdk that is not written to run in the browser, and hence needs the 'Node' builtins: "os", "path", "url", "buffer", "http", "https", "stream", "process" and "util".

As these are not defined in the browser, the web app crashed out with an undefined symbol.

For comparison here are the dependencies pulled in by Rollup for sdk version 3.6.0:

multi-entry.js:
amazon-chime-sdk-js - 1.58 MB (30.03%)
@fortawesome/free-solid-svg-icons - 1.21 MB (22.97%)
pdfjs-dist - 798.7 KB (14.82%)
app - 525.72 KB (9.76%)
pako - 425.18 KB (7.89%)
web-streams-polyfill - 196.09 KB (3.64%)
@fortawesome/fontawesome-svg-core - 107.86 KB (2.00%)
utif - 56.91 KB (1.06%)
jpeg-lossless-decoder-js - 52.77 KB (0.98%)
protobufjs - 51.24 KB (0.95%)
ua-parser-js - 43.24 KB (0.80%)
pepjs - 43.23 KB (0.80%)
resize-observer-polyfill - 32.87 KB (0.61%)
@aws-crypto/sha256-js - 31.73 KB (0.59%)
dicom-parser - 31.23 KB (0.58%)
dommatrix - 27.72 KB (0.51%)
rollup helpers - 23.86 KB (0.44%)
tslib - 22.03 KB (0.41%)
awesomplete - 12.76 KB (0.24%)
@protobufjs/float - 11.18 KB (0.21%)
@aws-crypto/util - 9.73 KB (0.18%)
detect-browser - 7.64 KB (0.14%)
@protobufjs/base64 - 3.85 KB (0.07%)
@protobufjs/utf8 - 3.29 KB (0.06%)
autosize - 3.17 KB (0.06%)
@aws-sdk/util-utf8-browser - 2.31 KB (0.04%)
@protobufjs/eventemitter - 2.04 KB (0.04%)
@protobufjs/aspromise - 1.55 KB (0.03%)
@protobufjs/pool - 1.21 KB (0.02%)
canvas - 1.04 KB (0.02%)
@aws-sdk/util-hex-encoding - 1.01 KB (0.02%)
@protobufjs/inquire - 544 B (0.01%)

As you can see there are a lot less @aws-sdk modules bundled in the build, and the ones that are included are written to be able to run in the browser.

Amazon Chime SDK for JavaScript version

3.7.0

What browsers are you seeing the problem on?

problem during build with Rollup.

Browser version

N/A

Meeting and Attendee ID Information.

N/A

Browser console logs

N/A

keean commented 2 years ago

Quick follow up, installing "rollup-plugin-polyfill-node" does not fix the issue, as there are properties used buy the aws-sdk that are not available in the polyfills. For example some of the AWS-SDK tries to access the local .aws credentials in the filesystem on startup... this is obviously never going to work in the web browser, but is also a big waste of space including all that code in a progressive web app. Rollup tries to tree-shake the imports, but it cannot tree-shake top level code that runs when the module loads, really a client side SDK like amazon-chime-sdk-js should not be loading modules that run server side code at the top level.

Really the module in the aws-sdk needs to be split in two so that code that can run on either the client or the server is in a separate file that can be imported independently of the server dependent stuff. However this was done in 3.6.0 worked fine, so it could be done that way. I suspect that there was an attempt to reduce code duplication by importing code from the aws-sdk into amazon-chime-sdk-js, but the way is has been done breaks bundling for browser use.

devalevenkatesh commented 1 year ago

Hello @keean ,

Thanks for reporting this issue. Could you please let us know how your rollup config looks like? We are trying to reproduce this issue on our side.

keean commented 1 year ago

import nodeResolve from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import multiEntry from '@rollup/plugin-multi-entry';
import typescript from 'rollup-plugin-typescript2';
import includePaths from 'rollup-plugin-includepaths';
import progress from 'rollup-plugin-progress';
import sizes from 'rollup-plugin-sizes';

export default [
{
        input: ['src/client.tsx', 'src/plugins/*.ts'],
        output: {
            format: 'iife',
            sourcemap: true,
            globals: {
                'zip-js': 'zip',
                'aws-sdk': 'AWS',
                'crypto': 'crypto',
            },
            name: 'client',
            file: 'build/client.js'
        },
        external: [
            'zip-js',
            'aws-sdk',
            'crypto',
        ],
        plugins: [
            multiEntry(),
            includePaths({
                include: {},
                paths: ['gen', 'static'],
                external: [],
                extensions: ['.js']
            }),
            nodeResolve({
                mainFields: ['module', 'browser', 'main'],
            }),
            progress(),
            typescript(),
            commonjs({
                include: [
                    'node_modules/**',
                    'static/**',
                    'gen/**',
                ],
            }),
            sizes({details: false})
        ],
    },
]

Note 1: we use 'multi-entry' to pick up plugins, so that the plugins get run and register themselves with the main code. This means a plugin just has to be dropped into the plugins directory, we don't have to change the code in the main file to import it.

Note 2: we include "gen" and "static". We use OpenAPI generate to produce typescript types for our own API from the declarative API spec which gets put in 'gen", and in "static" are some static data files.

We are using rollup v2.77.0, our source code is in Typescript, and these are the imports we use from the SDK:

import { ConsoleLogger, DefaultDeviceController, DefaultMeetingSession, LogLevel, MeetingSessionConfiguration, MeetingSession, AudioVideoObserver, VideoTileState, MeetingSessionStatus, DeviceChangeObserver, EventName, EventAttributes, MeetingSessionStatusCode, AudioProfile, DataMessage, ContentShareObserver } from 'amazon chime-sdk-js';

keean commented 1 year ago

A futher point is that Rollup does actually produce an output with the above config, its just that the output has the following undefined symbols:

path (guessing 'path')
fs (guessing 'fs')
url (guessing 'url')
buffer (guessing 'buffer')
http (guessing 'http')
https (guessing 'https')
stream (guessing 'stream')
process (guessing 'process$1')
child_process (guessing 'child_process')
util (guessing 'util$9')

keean commented 1 year ago

Here is a smaller version of the config, that still has the same error:

export default [{
    input: ['src/client.tsx'],
    output: {
        format: 'iife',
        sourcemap: true,
        name: 'client',
        file: 'build/client.js'
    },
    plugins: [
        nodeResolve({
            mainFields: ['browser'],
            preferBuiltins: true,
        }),
        commonjs(),
        json(),
        typescript(),
    ],
}]

LeviSklaroff commented 1 year ago

Hello @keean ,

So we were able to reproduce requiring @rollup/plugin-json when bundling the amazon-chime-sdk-js, however even after replicating your configuration we haven't been able to reproduce the other error you mention where it asks for rollup-plugin-polyfill-node you get after using the json plugin.

We were able to create a fix for requiring @rollup/plugin-json by changing how we're importing the package that was added, it's possible this might also fix your issue if you could try cloning it from the branch it's on https://github.com/aws/amazon-chime-sdk-js/tree/fixRollup
If that does not fix the issue for you, could you provide version info for the rollup plugins and any other unique build details.

keean commented 1 year ago

@LeviSklaroff I have a minimal package that replicates the problem the file structure is:

package.json rollup-config.js tsconfig.json src/test.ts

The contents of each file in order are:

{
  "name": "test",
  "private": true,
  "devDependencies": {
    "@rollup/plugin-commonjs": "^22.0.2",
    "@rollup/plugin-json": "^4.1.0",
    "@rollup/plugin-node-resolve": "^14.1.0",
    "rollup": "^2.79.1",
    "rollup-plugin-typescript2": "^0.34.0"
  },
  "dependencies": {
    "amazon-chime-sdk-js": "^3.8.0"
  }
}

import nodeResolve from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import typescript from 'rollup-plugin-typescript2';
import json from '@rollup/plugin-json';

export default [
    {
        input: ['src/test.ts'],
        output: {
            format: 'iife',
            sourcemap: true,
            name: 'client',
            file: 'build/client.js'
        },
        plugins: [
            nodeResolve({
                mainFields: ['browser'],
            }),
            commonjs(),
            json(),
            typescript(),
        ],
    },
]

{
    "compilerOptions": {
        "esModuleInterop": true,
        "strict": true,
        "strictNullChecks": true,
        "downlevelIteration": true,
        "allowSyntheticDefaultImports": true,
        "allowUnreachableCode": false,
        "noUnusedLocals": false,
        "noUnusedParameters": false,
        "noFallthroughCasesInSwitch": true,
        "noImplicitReturns": true,
        "removeComments": true,
        "target": "es2015",
        "module": "esnext",
        "moduleResolution": "node",
        "sourceMap": true,
        "noImplicitAny": true,
        "rootDir": ".",
        "outDir": "build",
        "baseUrl": "src"
    }
}

import { MeetingSession, AudioProfile } from 'amazon-chime-sdk-js';

export function test(meetingSession: MeetingSession) {
    meetingSession.audioVideo.setAudioProfile(AudioProfile.fullbandSpeechMono());
}

npm install
npx rollup -c rollup-config.js

will produce the following output:

src/test.ts → build/client.js...
(!) Missing shims for Node.js built-ins
Creating a browser bundle that depends on "os", "path", "url", "buffer", "http", "https", "stream", "process" and "util". You might need to include https://github.com/FredKSchott/rollup-plugin-polyfill-node
(!) Missing global variable names
Use output.globals to specify browser global variable names corresponding to external modules
crypto (guessing 'require$$0$3')
os (guessing 'require$$0$4')
path (guessing 'require$$1$5')
fs (guessing 'require$$0$5')
url (guessing 'require$$1$7')
buffer (guessing 'require$$1$6')
http (guessing 'require$$2')
https (guessing 'require$$3')
stream (guessing 'require$$0$6')
http2 (guessing 'require$$2$1')
process (guessing 'require$$2$2')
child_process (guessing 'require$$1$8')
util (guessing 'require$$2$3')
(!) Circular dependencies
node_modules/protobufjs/src/util/minimal.js -> node_modules/protobufjs/src/util/longbits.js -> node_modules/protobufjs/src/util/minimal.js
node_modules/amazon-chime-sdk-js/build/index.js -> node_modules/amazon-chime-sdk-js/build/audiovideocontroller/DefaultAudioVideoController.js -> node_modules/amazon-chime-sdk-js/build/task/PromoteToPrimaryMeetingTask.js -> node_modules/amazon-chime-sdk-js/build/index.js
node_modules/@aws-sdk/credential-provider-ini/dist-cjs/resolveProfileData.js -> node_modules/@aws-sdk/credential-provider-ini/dist-cjs/resolveAssumeRoleCredentials.js -> node_modules/@aws-sdk/credential-provider-ini/dist-cjs/resolveProfileData.js
(!) Use of eval is strongly discouraged
https://rollupjs.org/guide/en/#avoiding-eval
node_modules/@protobufjs/inquire/index.js
10: function inquire(moduleName) {
11:     try {
12:         var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
                      ^
13:         if (mod && (mod.length || Object.keys(mod).length))
14:             return mod;
created build/client.js in 5.1s

Here you can see there is a whole bunch of node dependencies "crypto, os, path, fs, url, buffer, http, https, stream, http2, process, child_process, util" that prevent the code working in the browser, and really should not be necessary. This works fine with Chime 3.6.0.

If I change the package.json to:

{
  "name": "test",
  "private": true,
  "devDependencies": {
    "@rollup/plugin-commonjs": "^22.0.2",
    "@rollup/plugin-json": "^4.1.0",
    "@rollup/plugin-node-resolve": "^14.1.0",
    "rollup": "^2.79.1",
    "rollup-plugin-typescript2": "^0.34.0"
  },
  "dependencies": {
    "amazon-chime-sdk-js": "3.6.0"
  }
}

then delete package-lock.json and the build and node_modules directories, and repeat the npm install and rollup, I get the "correct' output:

src/test.ts → build/client.js...
(!) Circular dependencies
node_modules/protobufjs/src/util/minimal.js -> node_modules/protobufjs/src/util/longbits.js -> node_modules/protobufjs/src/util/minimal.js
node_modules/amazon-chime-sdk-js/build/index.js -> node_modules/amazon-chime-sdk-js/build/audiovideocontroller/DefaultAudioVideoController.js -> node_modules/amazon-chime-sdk-js/build/task/PromoteToPrimaryMeetingTask.js -> node_modules/amazon-chime-sdk-js/build/index.js
(!) Use of eval is strongly discouraged
https://rollupjs.org/guide/en/#avoiding-eval
node_modules/@protobufjs/inquire/index.js
10: function inquire(moduleName) {
11:     try {
12:         var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
                      ^
13:         if (mod && (mod.length || Object.keys(mod).length))
14:             return mod;
created build/client.js in 3.3s

keean commented 1 year ago

The problem is still present in 3.8.0

keean commented 1 year ago

I think I have tracked it down to commit bce872c353edbb50908c5a0298f8113f1e8dcc82

c00656db07ecd9c04c3354418b7648bd2fa45707 works with Rollup.

keean commented 1 year ago

reverting this change:

diff --git a/src/messagingsession/DefaultMessagingSession.ts b/src/messagingsession/DefaultMessagingSession.ts
index 52c766ad..4fcd61cb 100644
--- a/src/messagingsession/DefaultMessagingSession.ts
+++ b/src/messagingsession/DefaultMessagingSession.ts
@@ -1,7 +1,7 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0

-import { GetMessagingSessionEndpointCommand } from '@aws-sdk/client-chime-sdk-messaging';
+//import { GetMessagingSessionEndpointCommand } from '@aws-sdk/client-chime-sdk-messaging';

 import FullJitterBackoff from '../backoff/FullJitterBackoff';
 import CSPMonitor from '../cspmonitor/CSPMonitor';
@@ -115,9 +115,9 @@ export default class DefaultMessagingSession implements MessagingSession {
           endpointUrl = (await this.configuration.chimeClient.getMessagingSessionEndpoint())
             .Endpoint.Url;
         } else {
-          endpointUrl = (
-            await this.configuration.chimeClient.send(new GetMessagingSessionEndpointCommand({}))
-          ).Endpoint.Url;
+          //endpointUrl = (
+            //await this.configuration.chimeClient.send(new GetMessagingSessionEndpointCommand({}))
+          //).Endpoint.Url;
         }
         this.logger.debug(`Messaging endpoint resolved to: ${endpointUrl}`);
       } catch (e) {

Gets it working again...

LeviSklaroff commented 1 year ago

Hi @keean ,

We've pinpointed the issue to @aws-sdk/client-chime-sdk-messaging dependency that was added. This is needed for important fix for DefaultMessagingSession and we're trying to investigate if we can find a fix that keeps this. To help you unblock could you try adding the @rollup/plugin-json, rollup-plugin-polyfill-node plugins and then also set preferBuiltins: false.

keean commented 1 year ago

@LeviSklaroff Unfortunately 'rollup-plugin-polyfill-node' does not provide all the needed methods:

[!] (plugin rpt2) Error: 'createHash' is not exported by polyfill-node.crypto.js, imported by ../amazon-chime-sdk-js/node_modules/@aws-sdk/shared-ini-file-loader/dist-es/getSSOTokenFilepath.js
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
../amazon-chime-sdk-js/node_modules/@aws-sdk/shared-ini-file-loader/dist-es/getSSOTokenFilepath.js (1:9)
1: import { createHash } from "crypto";
            ^
2: import { join } from "path";
3: import { getHomeDir } from "./getHomeDir";

    at error (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:198:30)
    at Module.error (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:12560:16)
    at Module.traceVariable (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:12919:29)
    at ModuleScope.findVariable (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:11571:39)
    at FunctionScope.findVariable (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:6503:38)
    at ChildScope.findVariable (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:6503:38)
    at Identifier.bind (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:7570:40)
    at CallExpression.bind (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:5400:23)
    at CallExpression.bind (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:9066:15)
    at VariableDeclarator.bind (/home/keean/Code/p4b-upgrade/node_modules/rollup/dist/shared/rollup.js:5400:23)

LeviSklaroff commented 1 year ago

@keean

So I was able to get it to build successfully off the build you provided with the following config

import nodeResolve from '@rollup/plugin-node-resolve'; import commonjs from '@rollup/plugin-commonjs'; import typescript from 'rollup-plugin-typescript2'; import json from '@rollup/plugin-json'; import polyfill from 'rollup-plugin-polyfill-node';

export default [ { input: ['src/test.ts'], output: { format: 'iife', sourcemap: true, name: 'client', file: 'build/client.js' }, plugins: [ nodeResolve({ browser: true, mainFields: ['browser'], preferBuiltins: false }), commonjs(), polyfill(), json(), typescript(), ], }, ]

However it seems by adding 'module' to mainfield like your application's config it fails to build. Could you test if it builds successfully either using the provided config or by removing the module mainfield?

keean commented 1 year ago

@LeviSklaroff

Getting rid of 'module' from 'mainFields' appears to fix the 'createHash' not found in crypto build problem.

It now builds with the above config, however it does not work, and fails as soon as the page loads in the browser with the following error:

getSSOTokenFromFile.js:6 Uncaught TypeError: Cannot destructure property 'readFile' of 'fs_1.promises' as it is undefined.
    at requireGetSSOTokenFromFile (getSSOTokenFromFile.js:6:9)
    at index.js:7:22
    at requireDistCjs$l (index.js:11:50)
    at requireFromIni (fromIni.js:4:34)
    at index.js:4:22
    at requireDistCjs$4 (index.js:4:52)
    at requireDefaultProvider (defaultProvider.js:5:35)
    at index.js:4:22
    at requireDistCjs$2 (index.js:4:60)
    at requireRuntimeConfig$1 (runtimeConfig.js:8:36)

It seems like one of the aws-sdk modules is trying to load the SSO token as a side-effect of importing the module.

keean commented 1 year ago

@LeviSklaroff so it looks like Rollup is including 'runtimeConfig.js' instead of 'runtimeConfig.browser.js' when it is bundling the @aws-sdk

Another concern though is how stuff is being pulled into the bundle because of one type and one function being used in amazon-chime-sdk-js

Really the aws-sdk seems not designed for tree-shaking using 'export *' which is bad because it forces everything to be bundled because of side-effects.

If every function call to the SDK pulls in this much code, a a simple 10 line application is going to take many megabytes to download. This might be okay with low-usage applications, but when you have millions of downloads all that bandwidth will add up.

LeviSklaroff commented 1 year ago

@keean Yes it looks like that's the issue, it seems rollup 14 has introduced a bug when being used with typescript that causes the browser flag to be ignored https://github.com/rollup/plugins/issues/1267 I was able to successfully bundle the application after downgrading to version 13. It does still require @rollup/plugin-json to bundle successfully but other then that I did not run into any issues using the mainfield: ['module', 'browser', 'main']. We are also aware of the size issue and are look into alternatives.

keean commented 1 year ago

@LeviSklaroff I have got the test-case to build with latest versions (delete node_modules and package-lock.json), it seems to pull in the correct runtimeConfig.browser.js now (adding "modules" before "browser" as I noticed that the aws-sdk only has the browser overloads for 'dist-es'.

The strange thing is the full application is still bundling runtimeConfig.js... so there is still something not quite right.

LeviSklaroff commented 1 year ago

@keean Hmm that's interesting that you were able to get the test case working but not the full application. All you did was delete node_modules and package-lock.json is that correct? Also were you able to test if it correctly bundles with version 13 of @rollup/plugin-node-resolve?

keean commented 1 year ago

@LeviSklaroff So I think I have tracked it down to the includePaths plugin "rollup-plugin-includepaths". Modifying the rollup.config.js to:

import nodeResolve from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import typescript from 'rollup-plugin-typescript2';
import json from '@rollup/plugin-json';
import sizes from 'rollup-plugin-sizes';
import includePaths from 'rollup-plugin-includepaths';

export default [
    {
        input: ['src/test.ts'],
        output: {
            format: 'iife',
            sourcemap: true,
            name: 'client',
            file: 'build/client.js'
        },
        plugins: [
            includePaths(),
            nodeResolve({
                mainFields: ['module', 'browser'],
            }),
            commonjs(),
            json(),
            typescript(),
            sizes({details: true}),
        ],
    },
]

Seems to make it all go wrong. We use includePaths to pull in some auto-generated code from tooling. We want to keep source and generated files separate, so the generated files directory can be deleted without losing any source code. To achieve this we put a .d.ts type file for the auto-generated code which is hand-written source code into the src directory. Then rollup pulls in the implementation from the gen directory. To make this work we use includePaths to add gen to the search path used when looking for modules.

keean commented 1 year ago

@LeviSklaroff I have fixed my issue by finding another way to resolve the typescript path aliases in rollup which does not use includePaths. I don't know why includePaths is preventing the correct version of runtimeConfig from being used, but it was also pulling in multiple copies of the same file, which made me think this may not be easy to fix, and it may be better to avoid includePaths altogether.

After about a week of trying to get it working, I think I have succeeded. The main problem was I needed to import a JavaScript file which had a .d.ts type file in a separate directory. The eventual solution was to make the path alias in the tsconfig file search both directories:

"paths": {
    "@gen/*": ["gen/*", "src/*"],
},

this allowed the import '@gen/validate' to find the src/validate.d.ts file in the src directory. Then I had to map this to something else for rollup, and I eventually got this working with the @rollup/plugin-alias like this:

alias({
    entries: [
        {find: /^@gen\/(.*)/, replacement: 'gen/$1.js'}
    ]
}),

which replaces @gen/validate with gen/validate.js finding the auto-generate js file in the gen directory.

devalevenkatesh commented 1 year ago

Thank you @keean for chasing this! Looks like a hard find resolution. Could you please summarize the issue and the fix with the includePaths. I went through this and see that this is related the rollup plugin or bundler itself? This would benefit the rollup bundler users if they are facing such as issue. So far, we have got this single issue, hence, checking whether this is issue with the Amazon Chime SDK for JavaScript using the messaging client SDK or this has workaround using rollup.

keean commented 1 year ago

@devalevenkatesh There were two (or three) problems:

for some reason when importing the aws-sdk the newer version of Rollup needs the @rollup/plugin-json before the typescript plugin in the rollup.config.js file.
Secondly there was a bug in rollup-plugin-typescript2 that got fixed with version 0.34.1
Finally I think there is a problem with the rollup-plugin-includepaths plugin that seems to prevent the correct runtimeConfig being loaded in the aws-sdk. I worked around this by finding another way to build the bundle that does not need rollup-plugin-includepaths.

nainkunal933 commented 1 year ago

@keean, A couple of questions about this fix. I will like to add some documentation for other builders who might run into the same issue.

What bug fix in rollup-plugin-typescript2 are you referring to? I looked at the changelog of 0.34.1 but did not see anything that would relate to your build errors.
In your alias setting, why is the alias source set to /^@gen\/(.*)/? aws-sdk-js build should not be under that alias. Is that alias for your other dependencies or for aws-sdk runtime config fix?

nainkunal933 commented 1 year ago

@keean I have raised a PR to update the FAQ guide. Please feel free to review the PR as you have context and knowledge about this issue.

Thank you for your efforts on this issue.

keean commented 1 year ago

@nainkunal933 probably best to ignore the specific fix, but the important bit is do not use "rollup-plugin-includepaths", the specific work around will depend on what includePaths was being used for.

As for the new version of the typescript2 plugin see this thread: https://github.com/rollup/plugins/issues/1267#issuecomment-1267981557

nainkunal933 commented 1 year ago

@keean That makes sense. I think that lines up with the FAQ I wrote. BTW please feel free to review my PR. I will appreciate your feedback since you have the most context on the issue. I will close this issue after merging the PR.

keean commented 1 year ago

@nainkunal933 There has been a regression at or before 3.10.0 where the rollup 'json' plugin is being required again.

nainkunal933 commented 1 year ago

@keean Can you please open a new ticket? We want to capture this possible regression in a separate issue. Since, JS SDK does not ship with a default rollup config please provide your rollup config for faster debugging.

Also, have you tried using our singlejs demo that is delivered in the aws-samples. Demo: https://github.com/aws-samples/amazon-chime-sdk/tree/main/utils/singlejs

This demo is using rollup with JS SDK's 3.10.0 version.

rapito commented 8 months ago

Just stumbled on this issue as well, and the docs do not mention rollup nor polyfills at all.

devalevenkatesh commented 8 months ago

Just stumbled on this issue as well, and the docs do not mention rollup nor polyfills at all.

Could you check https://aws.github.io/amazon-chime-sdk-js/modules/faqs.html#why-is-my-build-failing-after-upgrading-to-amazon-chime-sdk-for-javascript-370 and let us know if that helps?

rapito commented 8 months ago

Just stumbled on this issue as well, and the docs do not mention rollup nor polyfills at all.

Could you check https://aws.github.io/amazon-chime-sdk-js/modules/faqs.html#why-is-my-build-failing-after-upgrading-to-amazon-chime-sdk-for-javascript-370 and let us know if that helps?

That worked wonders, thank you!

For direct reference if anyone a me is using esbuild, until the esbuild issue referenced in the document above is fixed, add this to your esbuild config:

{
...
mainFields: ['browser','module', 'main'],
...
}

or

--main-fields=browser,module,main

aws / amazon-chime-sdk-js