To solve the npm package size issue #252

TingDaoK · 2021-09-09T00:43:22Z

Issue #, if available:

The current aws-crt npm package size is too huge(200 MB)

Description of changes:

Remove C source code from npm package
Publish C source code to S3 with publish pipeline, and the build script will download it when needed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…npm-package

lgtm-com · 2021-09-09T00:49:27Z

This pull request introduces 2 alerts when merging 83fb37d into e9bd55f - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

continuous-delivery/pack.sh

scripts/build.js

TingDaoK · 2021-09-11T00:52:28Z

scripts/build.js

+    // There is no native code, we are not building from source.
+    (async () => {
+        // AWS common runtime aws-crt-nodejs cloudfront distribution.
+        const url = "http://d332vdhbectycy.cloudfront.net/";


We can use github release asset instead of cloudfront deployment when we release. But, AWS is easier to test for now.

This reverts commit d86f783.

bretambrose

Seems reasonable but unless I'm missing something we should test this cloudfront-sourced build on all three platforms just as a sanity check.

package.json

TingDaoK · 2021-09-13T23:19:40Z

Seems reasonable but unless I'm missing something we should test this cloudfront-sourced build on all three platforms just as a sanity check.

It's a bit tricky to test it. What I did is download the packed tarball from npm pack(aws-crt-<version>.tgz), and test it by:

Remove the binary
Run node scripts/install.js locally.

I tested it on mac manually, and it works. I can test it on linux and windows for sure.

scripts/build.js

graebm · 2021-09-14T22:56:35Z

README.md

@@ -40,6 +40,8 @@ You can either add it to package.json (if using a tool like webpack), or just im
 npm install aws-crt
 ```

+To reduce the size of package, we put the C source code in the S3 bucket. If the platform you are using doesn't have the prebuilt binary, npm will pull the source from S3 bucket. In case, you don't have the public internet, you can specify the "CRT_SOURCE_CODE_HOST" environment variable for the host of the source code. The build script will fetch source code from that host instead. To fetch the source from S3, you can reach to the cloudfront distribution (Only works for version after v1.9.2) `https://d332vdhbectycy.cloudfront.net/aws-crt-<version>-source.tgz`, the sha256 checksum `https://d332vdhbectycy.cloudfront.net/aws-crt-<version>-source.sha256`


couple things:

Let's do "BINARY_HOST" instead of "SOURCE_CODE_HOST" since we know we'll be putting binaries there too soon enough. I mean, hey, .tgz files are binaries whether or not they have source code 😬

Looks like there is some precedent for letting users customize the host, and it's via the .npmrc file, with a variable like aws_crt_binary_host, instead of an environment variable

lol, I was confused by the "binary" as those ".node" files are things we talked about a lot.

scripts/build.js

graebm · 2021-09-14T23:02:21Z

scripts/build.js

+        const version = package["version"];
+        fetch_native_code(url, version, tmp_path).then(() => {
+            // Clean up temp directory
+            fs.rmSync(tmp_path, { recursive: true });


to be improved in the future: probably better to cache the download, but then we'd need integrity checks to ensure the cached version is good, so this works for now. Here's prebuild-install explaining their caching strategy

package.json

scripts/build.js

lgtm-com · 2021-09-15T19:17:36Z

This pull request introduces 1 alert when merging 967aa2f into e9bd55f - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

AllanZhengYP

Looks good generally, left a few comments

README.md

continuous-delivery/pack.sh

AllanZhengYP · 2021-09-15T22:37:25Z

continuous-delivery/pack.sh

+# now get the tag
+CURRENT_TAG=$(git describe --tags | cut -f2 -dv)
+# convert v0.2.12-2-g50254a9 to 0.2.12
+CURRENT_TAG_VERSION=$(git describe --tags | cut -f1 -d'-' | cut -f2 -dv)


May be check if the tagged commit at current head? This can prevent updating binaries without bumping versions.

I removed a check that was there for testing. Added it back to check if the tag is release. So, in that case, we can prevent updating binaries without proper release, which will also prevent the version is not updated, I think.

AllanZhengYP · 2021-09-15T22:39:59Z

package.json

@@ -47,6 +47,7 @@
    "crypto-js": "^4.0.0",
    "fastestsmallesttextencoderdecoder": "^1.0.22",
    "mqtt": "^4.2.8",
+    "tar": "^6.1.11",


We saw customer complaining of the deprecated dependencies too, are those tackled already?
aws/aws-sdk-js-v3#2750

I saw those warnings as well, don't know how to solve them... npm update and npm audit fix is not helping... But, we can probably tackle them after

scripts/build.js

README.md

scripts/build.js

TingDaoK and others added 4 commits September 8, 2021 17:41

I am a bad js programer

1974727

Merge branch 'main' into npm-package

f371350

add one more todo

e477526

Merge branch 'npm-package' of github.com:awslabs/aws-crt-nodejs into …

83fb37d

…npm-package

TingDaoK added 2 commits September 10, 2021 15:25

TODO checksum

4b001af

Checksum

a46bbaf

TingDaoK commented Sep 11, 2021

View reviewed changes

remove test line

6b58845

TingDaoK marked this pull request as ready for review September 11, 2021 00:53

TingDaoK added 5 commits September 10, 2021 17:58

check the path is correct

bdcb541

just for testing

d86f783

Revert "just for testing"

7a84ef3

This reverts commit d86f783.

pro test

fd26fb1

you don't need typedoc

d37cc1e

TingDaoK requested a review from bretambrose September 13, 2021 20:54

bretambrose reviewed Sep 13, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

use ^ to include any newer version

8630a0c

graebm reviewed Sep 13, 2021

View reviewed changes

scripts/build.js Outdated Show resolved Hide resolved

TingDaoK added 5 commits September 14, 2021 14:31

sha 256 instead

0a294d3

Still have the checksum at S3

465e5ed

introduce the env variable and README

7e17e3e

markdown

e7acc86

get rid of the newline for the checksums

8f865de

graebm reviewed Sep 14, 2021

View reviewed changes

TingDaoK added 4 commits September 14, 2021 18:05

Coments addressed, first round

338950e

our own copy folder and firendly change work directory

30f62db

remove test code and be more like a JS developer

ebf6066

more styling fix

2a4a334

TingDaoK added 5 commits September 15, 2021 11:05

remove uuid as well

429486a

node build-in modules first

0683a23

remove source code after build

3bca139

again, forgot to remove the test

410ca2f

also another one for test

28bed15

TingDaoK requested a review from AllanZhengYP September 15, 2021 18:34

don't need to copy around

967aa2f

TingDaoK added 2 commits September 15, 2021 13:05

remove the unused variable

5ed6f0c

tweak, return 0 at end

f8240a2

AllanZhengYP reviewed Sep 15, 2021

View reviewed changes

graebm reviewed Sep 15, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

scripts/build.js Outdated Show resolved Hide resolved

scripts/build.js Outdated Show resolved Hide resolved

TingDaoK added 5 commits September 16, 2021 10:30

better error handling

58ba0b1

check if it's a release

95714c8

comments addressed

b63392c

to fix npm warning?

9391224

npm update

32a4c82

graebm approved these changes Sep 16, 2021

View reviewed changes

TingDaoK merged commit 03a6dd7 into main Sep 16, 2021

TingDaoK deleted the npm-package branch September 16, 2021 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

To solve the npm package size issue #252

To solve the npm package size issue #252

TingDaoK commented Sep 9, 2021 •

edited

Loading

lgtm-com bot commented Sep 9, 2021

TingDaoK Sep 11, 2021

bretambrose left a comment

TingDaoK commented Sep 13, 2021 •

edited

Loading

graebm Sep 14, 2021

TingDaoK Sep 15, 2021

graebm Sep 14, 2021

lgtm-com bot commented Sep 15, 2021

AllanZhengYP left a comment

AllanZhengYP Sep 15, 2021

TingDaoK Sep 16, 2021

AllanZhengYP Sep 15, 2021

TingDaoK Sep 16, 2021

To solve the npm package size issue #252

To solve the npm package size issue #252

Conversation

TingDaoK commented Sep 9, 2021 • edited Loading

lgtm-com bot commented Sep 9, 2021

Choose a reason for hiding this comment

bretambrose left a comment

Choose a reason for hiding this comment

TingDaoK commented Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 15, 2021

AllanZhengYP left a 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

TingDaoK commented Sep 9, 2021 •

edited

Loading

TingDaoK commented Sep 13, 2021 •

edited

Loading