-
Notifications
You must be signed in to change notification settings - Fork 470
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
3.7.0 broke build with Rollup. #2455
Comments
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. |
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. |
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:
|
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:
|
Here is a smaller version of the config, that still has the same error:
|
Hello @keean , So we were able to reproduce requiring
|
@LeviSklaroff I have a minimal package that replicates the problem the file structure is: package.json The contents of each file in order are:
will produce the following output:
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:
then delete package-lock.json and the build and node_modules directories, and repeat the npm install and rollup, I get the "correct' output:
|
The problem is still present in 3.8.0 |
reverting this change:
Gets it working again... |
Hi @keean , We've pinpointed the issue to |
@LeviSklaroff Unfortunately 'rollup-plugin-polyfill-node' does not provide all the needed methods:
|
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'; export default [ 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 |
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:
It seems like one of the aws-sdk modules is trying to load the SSO token as a side-effect of importing the module. |
@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. |
@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 rollup/plugins#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. |
@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. |
@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 |
@LeviSklaroff So I think I have tracked it down to the includePaths plugin "rollup-plugin-includepaths". Modifying the rollup.config.js to:
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 |
@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
this allowed the
which replaces |
Thank you @keean for chasing this! Looks like a hard find resolution. Could you please summarize the issue and the fix with the |
@devalevenkatesh There were two (or three) problems:
|
@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.
|
@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. |
@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: rollup/plugins#1267 (comment) |
@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. |
@nainkunal933 There has been a regression at or before 3.10.0 where the rollup 'json' plugin is being required again. |
@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 This demo is using |
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
|
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?
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:
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:
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
The text was updated successfully, but these errors were encountered: