Bundle jsii modules as npm tarballs #52

eladb · 2018-07-03T20:59:40Z

This change modifies the way we bundle jsii modules. Instead of webpacking them
and storing the code inside the assembly spec, we now simply use "npm pack" to
produce an npm tarball (.tgz) and include that in the generated library.

The kernel now simply accepts the path to the tarball (as oppose to the entire
code passed through STDIN), and untars it into a working directory (under node_modules).
This effectively allows dependencies to require(it).

Bundled dependencies are now simple npm bundled dependencies. If a module uses the old-
style configuration, we fail with instructions.

PERF: This change also improves load-time performance (again, we are not sending
a big JSON file over the wire, parse it, and then evaluate it into the VM).

.NET: Furthermore, in .NET we didn't even bundle the webpack and the jsii-runtime, so this change includes modifications to the .NET generator and runtime to actually bundle the runtime and the new tarballs. This removes the dependency on jsii-runtime for .NET, and leaves only node.js as an external dependency.

NOTE ABOUT VERSIONS: we currently do not support multiple versions of the same module loaded together into the same kernel space. This is a major and unacceptable limitation for production
environments, which we must address, or otherwise, people will constantly hit these
restrictions as jsii software stacks evolve.

BREAKING CHANGE

This change modifies the way we bundle jsii modules. Instead of webpacking them and storing the code inside the assembly spec, we now simply use "npm pack" to produce an npm tarball (.tgz) and include that in the generated library. The kernel now simply accepts the path to the tarball (as oppose to the entire code passed through STDIN), and untars it into a working directory (under node_modules). This effectively allows dependencies to `require(it)`. This change also dramatically improves load-time performance (again, we are not sending a big JSON file over the wire, parse it, and then evaluate it into the VM). Bundled dependencies are now simple npm bundled dependencies. If a module uses the old- style configuration, we fail with instructions. NOTE: we currently do not support multiple versions of the same module loaded together into the same kernel space. This is a major and unacceptable limitation for production environments, which we must address, or otherwise, people will constantly hit these restrictions as jsii software stacks evolve. BREAKING CHANGE.

mpiroc · 2018-07-03T21:25:52Z

.gitignore

@@ -348,6 +348,7 @@ jspm_packages/

 # Output of 'npm pack'
 *.tgz
+!*.jsii.tgz


Do we really want to check in tarballs? Why not ignore them at the .npmignore level instead?

No, thats obviously a mistake

mpiroc · 2018-07-03T21:34:09Z

packages/jsii-kernel/lib/api.ts

@@ -27,7 +27,17 @@ export interface HelloResponse {
 }

 export interface LoadRequest {
-    assembly: Assembly;
+    /** @deprecated */
+    assembly?: Assembly;


Since we throw an error if assembly is present, is deprecated the right semantics? I think it would be better to remove the assembly property altogether (and continue to throw an error if it is present).

mpiroc · 2018-07-03T21:34:51Z

packages/jsii-kernel/lib/kernel.ts

@@ -211,7 +274,9 @@ export class Kernel {
            return this._wrapSandboxCode(() => fn.apply(obj, this._toSandboxValues(args)));
        });

-        this._debug('method returned:', ret);
+        // console.error(ret);


Remove commented code

mpiroc · 2018-07-03T21:37:43Z

packages/jsii-pacmak/lib/generator.ts

@@ -47,41 +47,64 @@ export interface IGenerator
 */
 export abstract class Generator implements IGenerator {
    private readonly options: GeneratorOptions;
-    private readonly mod: spec.Assembly
+    private mod: spec.Assembly


Nit: More readable to use the full word module. More broadly, it isn't clear what the difference is between modules and assemblies. If there is no difference, we should use the same terminology everywhere.

Got rid of .mod

mpiroc · 2018-07-03T21:39:43Z

packages/jsii-pacmak/lib/generator.ts

    /**
     * Runs the generator (in-memory).
     */
    generate() {
        this.onBeginAssembly(this.mod);
-        this.visit(this.mod.nametree);
+        if (this.mod.nametree) {
+            this.visit(this.mod.nametree);


I thought we deprecated nametree. If not, we should update http://sapp.amazon.com/jsii/.

Yes, I need to update the spec :-)
[please try to avoid using non-public URLs here]

mpiroc · 2018-07-03T21:42:15Z

update-version.sh

@@ -1,4 +1,5 @@
 #!/bin/bash
+set -e


Should this be set -euo pipefail?

Bundle jsii tarball modules into generated .NET packages and also bundle the jsii-runtime program as an embedded resource into the dotnet runtime library. This removes the dependency on jsii-runtime, and leaves only node.js as an external dependency. It also improves load-time performance since the tarball is only passed as a path to the runtime/kernel and not the entire .js code.

`set -euo` fails for unbound variables

Since we have version numbers in test expectations every build will get a different version number and that invariably causes tests to fail.

1. Add calc-lib 2. Create expected tarball on-the-fly to avoid binary diffs resulting from different platforms

RomainMuller · 2018-07-11T09:28:45Z

packages/jsii-pacmak/lib/program.ts


-    const generatorClass: any = await lookupGenerator(lang);
-    return new generatorClass(mod);
+    const tarball = await npmPack(packageDir);


Why do we always pack? Is this relevant, for example when we generate docs?

RomainMuller · 2018-07-11T09:29:24Z

packages/jsii-pacmak/lib/program.ts

-    await generator.save(outDir);
-}
+async function npmPack(packageDir: string) {
+    const child = spawn('npm', [ 'pack', '--ignore-scripts' ], { cwd: packageDir, stdio: [ 'ignore', 'pipe', 'pipe' ] });


I'm not sure this should --ignore-scripts here; as this no longer guarantees reproductible outcome.

Good point. I need to think about it, also how this interplays with cross-language builds and publishing (#73)

RomainMuller · 2018-07-11T09:30:52Z

packages/jsii-pacmak/lib/program.ts

+    const tarball = await new Promise<string>((ok, fail) => {
+        let stdout = '';
+        let stderr = '';
+        child.stdout.on('data', chunk => stdout += chunk.toString());


This is unsafe to do: the chunks may include incomplete multi-byte characters (the rest could be in the next chunk), which could render the stdout and stderr strings invalid.

Mmm.. Good to know. I will change to Buffer.concat.

RomainMuller · 2018-07-11T09:31:40Z

packages/jsii-pacmak/lib/program.ts

+                return ok(stdout.trim());
+            } else {
+                process.stderr.write(stderr);
+                return fail(new Error('Exit with status ' + status));


status might be null, if the child is killed by a signal.

eladb requested review from RomainMuller and mpiroc July 3, 2018 20:59

mpiroc approved these changes Jul 3, 2018

View reviewed changes

Elad Ben-Israel added 21 commits July 3, 2018 14:59

CR fixes

42c0d07

Merge branch 'benisrae/tarballs' into benisrae/dotnet-tarballs

7645943

Bring in a few fixes from dotnet-tarballs

031a2c0

Merge branch 'benisrae/tarballs' into benisrae/dotnet-tarballs

3678f06

Initial set of CR comments

959befd

Fix "update-version.sh"

c05c9b1

`set -euo` fails for unbound variables

Add CodeBuild build badge

13ff227

Merge branch 'benisrae/fix-update-version' into benisrae/dotnet-tarballs

84ef674

Merge branch 'benisrae/fix-update-version' into benisrae/tarballs

b9d3149

Add missing tgz to pacmak tests

d6f5e0d

Ignore +build when setting jsii version

d04f0b9

update versions

0c2e631

Require native type

e862a4a

Do not include commit sha in version

ff76775

Since we have version numbers in test expectations every build will get a different version number and that invariably causes tests to fail.

Improvements to pacmak tests

0efe687

1. Add calc-lib 2. Create expected tarball on-the-fly to avoid binary diffs resulting from different platforms

Take only first 7 chars from commit sha

0109d7c

Merge branch 'benisrae/dotnet-tarballs' into benisrae/tarballs

e358e35

Merge remote-tracking branch 'origin/master' into benisrae/tarballs

6672bc2

Add node_modules/.bin to PATH so we can execute lerna

05540b7

Fix a couple of tests

817c11b

eladb merged commit 0db8802 into master Jul 11, 2018

eladb deleted the benisrae/tarballs branch July 11, 2018 09:13

RomainMuller reviewed Jul 11, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundle jsii modules as npm tarballs #52

Bundle jsii modules as npm tarballs #52

eladb commented Jul 3, 2018 •

edited

Loading

mpiroc Jul 3, 2018

eladb Jul 3, 2018

mpiroc Jul 3, 2018

mpiroc Jul 3, 2018

mpiroc Jul 3, 2018

eladb Jul 3, 2018

mpiroc Jul 3, 2018

eladb Jul 3, 2018

mpiroc Jul 3, 2018

eladb Jul 3, 2018

RomainMuller Jul 11, 2018

eladb Jul 12, 2018 •

edited

Loading

RomainMuller Jul 11, 2018

eladb Jul 12, 2018

RomainMuller Jul 11, 2018

eladb Jul 12, 2018

RomainMuller Jul 11, 2018

Bundle jsii modules as npm tarballs #52

Bundle jsii modules as npm tarballs #52

Conversation

eladb commented Jul 3, 2018 • edited Loading

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

eladb Jul 12, 2018 • edited Loading

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

eladb commented Jul 3, 2018 •

edited

Loading

eladb Jul 12, 2018 •

edited

Loading