Skip to content
This repository has been archived by the owner on Nov 23, 2021. It is now read-only.

Consolidate bundling #41

Closed

Conversation

bookmoons
Copy link
Contributor

Combines external modules into a single compatibility layer bundle at convert time. Only bundles modules used in the output script. This should eliminate redundant code from shared dependencies of external modules.

Runs tinyify on the final bundle to optimize.

Closes #25.

@robingustafsson
Copy link
Member

Looks good overall. I run into an issue though when I have collections with scripts that are using the shim'ed libs, eg. expect.js (https://github.com/bookmoons/postman-to-k6/blob/bookmoons/consolidate-bundling/lib/shim/expect.js). Then I get this error:
GoError: stat /<removed>/postman-to-k6/libs/chai.js: no such file or directory
The output script is piped to /<removed>/postman-to-k6/. Looks like the libs in https://github.com/bookmoons/postman-to-k6/blob/bookmoons/consolidate-bundling/lib/shim/ are trying to load unbundled files.

@bookmoons
Copy link
Contributor Author

Shoot, will look into this.

@hiqqs
Copy link

hiqqs commented Sep 19, 2019

This will remove the need for k6 scripts to import the library shims / not have those in the same repo correct?

@robingustafsson
Copy link
Member

@johnhiggs No, this PR is for all shims and external libs to be put into one JS file rather than several different ones. It will also only include the external libs actually needed to run the output script, rather than all of them as is currently the case. Removing the need for external libs will require adding native support in k6 for these libs/APIs or by adding the relevant libs as curated libs to https://jslib.k6.io/ (can be used as remote modules or downloaded for use as local modules). The latter being the path we're currently aiming for.

@hiqqs
Copy link

hiqqs commented Sep 20, 2019 via email

@bookmoons
Copy link
Contributor Author

Went over this more carefully. It has the problem you found and it was also not quite bundling everything. Pushed fixes for all of this.

But this has revealed an issue. One of the dependencies uses Object.setPrototypeOf(), which seems to be unavailable in k6. Going to look into this.

@robingustafsson
Copy link
Member

Great, now it works better! 🎉

Regarding Object.setPrototypeOf(), maybe this would need an update of the core-js version we're using in k6. We've been trying to update core-js (and Babel.js) for some time now in k6, but I believe we ran into some issues with it. Let me check with the k6 team and I'll get back to you. If the update of core-js in k6 will take some time maybe we could polyfill ([1] or [2]) Object.setPrototypeOf() in postman-to-k6?

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf#Polyfill
[2] https://github.com/wesleytodd/setprototypeof

@bookmoons
Copy link
Contributor Author

Nice, yes. Maybe that could also eliminate that rough edge in the shim, where it couldn't quite get a clean prototype.

@bookmoons
Copy link
Contributor Author

Opened #43 for the prototype issue.

@bookmoons
Copy link
Contributor Author

Made a submission that should resolve the prototype issue #44. After that's settled I can rebase this patch onto it.

@bookmoons bookmoons force-pushed the bookmoons/consolidate-bundling branch from 732a69a to 8dbf1bf Compare October 11, 2019 16:15
@bookmoons
Copy link
Contributor Author

Rebased this onto the prototype fix. Tests pass. The polyfill now ends up bundled in the compat layer. This collection that loads all shim components converts and executes successfully. I think it's in good shape.

{
	"info": {
		"_postman_id": "9b0cc8d3-04d1-4196-87a0-3af92541f760",
		"name": "Depends",
		"schema": "https://schema.getpostman.com/json/collection/v2.1.0/collection.json"
	},
	"item": [
		{
			"name": "TestDepends",
			"event": [
				{
					"listen": "prerequest",
					"script": {
						"id": "f3c1e91d-0c0c-481f-b841-a5d067d40a48",
						"exec": [
							""
						],
						"type": "text/javascript"
					}
				},
				{
					"listen": "test",
					"script": {
						"id": "d7a05bac-89c9-4c48-aadc-472bc6631e6d",
						"exec": [
							"const cheerio = require('cheerio')",
							"const _ = require('lodash')",
							"const cryptoJs = require('crypto-js')",
							"",
							"const json = xml2Json(\"<html></html>\")",
							"pm.test('test body', function () {",
							"    pm.expect('Response body').to.include('Response')",
							"    pm.response.to.have.jsonSchema({})",
							"})",
							""
						],
						"type": "text/javascript"
					}
				}
			],
			"request": {
				"auth": {
					"type": "awsv4",
					"awsv4": [
						{
							"key": "secretKey",
							"value": "secret",
							"type": "string"
						},
						{
							"key": "accessKey",
							"value": "key",
							"type": "string"
						}
					]
				},
				"method": "GET",
				"header": [],
				"url": {
					"raw": "{{protocol}}://example.com?Action=List",
					"protocol": "{{protocol}}",
					"host": [
						"example",
						"com"
					],
					"query": [
						{
							"key": "Action",
							"value": "List"
						}
					]
				}
			},
			"response": []
		}
	],
	"variable": [
		{
			"id": "6148a217-db3c-4c2d-9807-992aab6ec48b",
			"key": "protocol",
			"value": "https",
			"type": "string"
		}
	],
	"protocolProfileBehavior": {}
}

@robingustafsson
Copy link
Member

Yup, this works now. I have tested it as well 👍

I'd like to see one improvement though. Right now this is what the import list looks like in the output script from the Postman collection you posted above:

import "./libs/shim/core.js";
import "./libs/shim/cheerio.js";
import "./libs/shim/crypto-js.js";
import "./libs/shim/lodash.js";
import "./libs/shim/expect.js";
import "./libs/shim/jsonSchema.js";
import "./libs/shim/xml2Json.js";
import "./libs/shim/urijs.js";
import { aws4, URI } from "./libs/compat.js";

and all the shims (well, except libs/shim/core.js) follow this pattern:

import { URI } from '../compat.js'
const Extend = Symbol.for('extend')
postman[Extend].module.urijs = URI

could we instead inject these 3 line glue code snippets into libs/shim/core.js (an renaming the shim layer libs/shim.js)? So injecting something like this:

// inject as imports
import { cheerio, cryptoJS, lodash, expect, jsonSchema, xml2json, URI } from '../compat.js'

// this line would be redundant in libs/shim/core.js
const Extend = Symbol.for('extend')

// inject these at an appropriate place after Extend is defined
postman[Extend].module.cheerio = cheerio
postman[Extend].module.cryptoJS = cryptoJS
postman[Extend].module.lodash = lodash
postman[Extend].module.expect = expect
postman[Extend].module.jsonSchema = jsonSchema
postman[Extend].module.xml2json = xml2json
postman[Extend].module.urijs = URI

This would mean the libs output would only be 2 files, libs/shim.js and libs/compat.js, rather than all the small libs/shim/*.js files that are being output right now.

@bookmoons
Copy link
Contributor Author

Will have a look at this.

@bookmoons
Copy link
Contributor Author

As discussed offthread, I'll consolidate the shim to get all external deps into a single file.

@bookmoons
Copy link
Contributor Author

I'm hitting issues here that are not yielding easily. There's some kind of interaction that seems to hit a memory leak. I'll have to put this aside for now.

@bookmoons bookmoons closed this Nov 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidated bundling [bounty: $250]
3 participants