Drop some amount of corejs also the file in git tree #1772

mstoykov · 2020-12-15T15:50:17Z

Reasons for doing it this way:

this lets us easily see what is actually included in the fina core-js
build by looking at k6-shim.js. The format there is probably going to
change as it is copied from the core-js repo's shim.js, but the idea
will stay
There is no need to have the shim.min.js in the git tree as it is
already in lib/rice-box.go. Maybe after the golang embed proposal is
available we can switch around, but this will likely take more then a
year before we don't need to support a version of golang without it
go generate is awesome, but in this case it has way too many things
that need to happen which makes it IMO a bad fit. This plus the fact
that this will not be changed regularly means it is probably better
this way.

Reasons for doing it this way: 1. this lets us easily see what is actually included in the fina core-js build by looking at k6-shim.js. The format there is probably going to change as it is copied from the core-js repo's shim.js, but the idea will stay 2. There is no need to have the shim.min.js in the git tree as it is already in lib/rice-box.go. Maybe after the golang embed proposal is available we can switch around, but this will likely take more then a year before we don't need to support a version of golang without it 3. go generate is awesome, but in this case it has way too many things that need to happen which makes it IMO a bad fit. This plus the fact that this will not be changed regularly means it is probably better this way.

mstoykov · 2020-12-15T16:12:25Z

Some performance running

import http from "k6/http";
import { sleep } from "k6";

export let options = {
    discardResponseBodies: true,
}
export default function() {
    http.get("https://httpbin.test.loadimpact.com/get");
    sleep(Math.random() * 2 + 3);
}

with -vus 500 --duration 60s main.js -v --no-thresholds --no-summary

version	iterations	mem usage	CPU usage	time to initialize VUs	run time
`15e15e5`	7464	1424300	45%	~5.8s	1:10.90
`0e6c0ab`	7472	599880	24%	~3.2s	1:07.19
`0e6c0ab` + base	7473	279292	18%	~0.9s	1:05.81

The first is ( the current ) master, 0e6c0ab is this PR and +base is with compatility-mode=base and the same script rewritten to be es5.

mstoykov · 2020-12-15T16:46:04Z

Comment on the still available core-js modules:

es7.array.flat-map - this seems fine and probably something that can be added to goja by us
es7.array.flatten - same as above + it is now called flat and flatten doesn't exist see https://github.com/tc39/proposal-flatMap
es7.string.trim-left, trim-right are synonims (from AnnexB(b for browser)) to trimStart,trimEnd - again can be added to goja (even easier) and will probably be welcomed as they are used by babel7 for some reason 🤦
es7.string.match-all - can be added and probably won't be all that hard, but we will need some benchmark tests ;)
es7.object.get-own-property-descriptors - the variant without s is in goja, so adding this shouldn't be too hard.
es7.object.values, es7.object.entries - no idea but seems easy to be added
es7.object.define-getter, es7.object.define-setter, es7.object.lookup-getter,es7.object.lookup-setter - apparently deprecated, so maybe not even worth implementing 🤷
es7.reflect.define-metadata and friends are seem to be https://github.com/rbuckton/reflect-metadata which isn't even a proposal AFAIK ... https://medium.com/jspoint/introduction-to-reflect-metadata-package-and-its-ecmascript-proposal-8798405d7d88 So maybe dropping them is a good idea as well

imiric · 2020-12-15T17:01:13Z

js/bundle_test.go

+		compatibilityTestSrc := `
+		module.exports.default = function() {};
+		if ([[1,2],[3,4]].flatten()[4]  == 4) {
+			throw Error("Array.flatten doesn't work")


This would never fail, no? It would always be undefined, even if flatten() works, unless you meant [3] === 4. Ah, nevermind, the index doesn't actually matter. :)

But this would be better checked with if (typeof Array.prototype.flatten !== 'function') ....

Also maybe we should use a different object to test this, since flatten was renamed to flat in corejs v3, and it would avoid us having to update this when we update to v3.

But this would be better checked with if (typeof Array.prototype.flatten !== 'function') ....

We should have both of these. First check if it's actually a function, and fast fail if it's no, then whether the behavior is at least somewhat along the lines of what one could expect.

Also maybe we should use a different object to test this, since flatten was renamed to flat in corejs v3, and it would avoid us having to update this when we update to v3.

Array.prototype.flat is the actual name according to the specification. We should probably use that name as well.

imiric

LGTM, just some minor suggestions.

I like that we can easily see and change what's included in corejs 👍

The idea is that we would run build.sh manually whenever we want to upgrade, right?

js/lib/build.sh

imiric · 2020-12-15T17:14:22Z

js/lib/corejs-build.sh

@@ -0,0 +1,5 @@
+#!/bin/sh
+
+npm i .


This is failing for me when I run build.sh. grunt fails to install because of:

npm ERR! code ERESOLVE npm ERR! ERESOLVE unable to resolve dependency tree npm ERR! npm ERR! While resolving: [email protected] npm ERR! Found: [email protected] npm ERR! node_modules/grunt npm ERR! dev grunt@"1.0.x" from the root project npm ERR! npm ERR! Could not resolve dependency: npm ERR! peer grunt@"~0.4" from [email protected] npm ERR! node_modules/grunt-livescript npm ERR! dev grunt-livescript@"0.6.x" from the root project npm ERR! npm ERR! Fix the upstream dependency conflict, or retry npm ERR! this command with --force, or --legacy-peer-deps npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

npm i --force works, but maybe there's a better workaround.

Hm ... that seems to be because I don't specify a version for node and it works with mine (14.x something) while it doesn't work with 15.4.0

14.x is still the LTS, so as long as it works with that, I wouldn't bother. Usually, current, contains weird kinks until it reaches lts.

imiric · 2020-12-15T18:28:27Z

Also, that's an impressive performance gain! 👏 🎉

How did you know what to remove and what to keep? Can we assure that we're not removing something a script was relying on?

mstoykov · 2020-12-16T08:24:57Z

@imiric mostly the discussion here dop251/goja#180 and talking with @simskij ... a long time ago. Plus obviously going through the things in the list from core-js v2.5.1 and looking at them, the test262 also helps ;)

Some of the drops might be used but also aren't standard at all ... just core-js used to (and probably still does) stuff that are still in the proposal process 🤦 - see the comments in k6-shim.js for it

Co-authored-by: Ivan Mirić <[email protected]>

imiric · 2020-12-16T09:58:39Z

js/lib/corejs-build.sh

+#!/bin/sh
+
+npm i --force .
+modules="$(cat shim.js | grep ^require  | sed "s|require('\./modules/||g"  | sed "s/');//g" | tr "\n" ",")"


If I'm not mistaken, you could simplify this to:

Suggested change

modules="$(cat shim.js | grep ^require | sed "s|require('\./modules/||g" | sed "s/');//g" | tr "\n" ",")"

modules="$(sed -n "s:^require('\./modules/$.*$');:\1:p" shim.js | paste -sd',')"

Also see useless use of cat ;)

~~Errm I messed up the quoting there so you'll need to escape it properly, but the command should work.~~ Nevermind, it works fine with /bin/sh, just tested it.

I am not going to this for now, as if we remove more from the shim.js or change its format, I will need to redo it.
For example, if we are finally left with 5-10 items I am probably just going to hard them inside the script instead of having a file with an explanation on what is what and why ;)

OK, though I don't see a reason why not to simplify this anyway.

simskij

Once @imiric's comments have been resolved. :)

js/lib/k6-shim.js

simskij · 2021-01-05T18:02:09Z

es7.array.flatten - same as above + it is now called flat and flatten doesn't exist see https://github.com/tc39/proposal-flatMap

Yeah, we should drop this, once it's in goja. Couldn't find any mention of it in dop251/goja#180 or by lazily searching the codebase from the GH UI.

es7.reflect.define-metadata and friends are seem to be rbuckton/reflect-metadata which isn't even a proposal AFAIK ... medium.com/jspoint/introduction-to-reflect-metadata-package-and-its-ecmascript-proposal-8798405d7d88 So maybe dropping them is a good idea as well

Yeah, if we ever get into any issues because of this, we can add it back or publish an article on how to polyfill it. My bet is that it won't ever happen.

es7.object.define-getter, es7.object.define-setter, es7.object.lookup-getter,es7.object.lookup-setter - apparently deprecated, so maybe not even worth implementing 🤷

Agreed. To sum it up, with some quite minor additions to goja, we should be able to drop the core-js shim layer altogether. 🎉

mstoykov · 2021-01-28T10:01:00Z

As @simskij eluded we have agreed to drop the last two points of #1772 (comment) :

the metadata one, because it is not in the spec, or even in the process of getting in. Our implementation is 4 years old so it likely has changed, but also is very unlikely to be used by anyone inside k6
the define/lookup-getter/setter because they have been deprecated for around a decade from what I could find and again are unlikely to be used.
Both of those can be polyfilled by people who need them in the cases where they do, so even if some users have used them for some reason, there is an easy fix for them.

Everything else has now been implemented in goja and as such we are dropping the whole corejs in #1824, which should land in v0.31.0.

And as such, I am closing this PR as it superseded.

mstoykov added this to the v0.30.0 milestone Dec 15, 2020

mstoykov requested review from imiric, simskij and na-- December 15, 2020 15:50

This was referenced Dec 15, 2020

Corejs rework pat2 #1735

Closed

PoC loading of specific parts of core-js #1732

Closed

imiric reviewed Dec 15, 2020

View reviewed changes

imiric suggested changes Dec 15, 2020

View reviewed changes

mstoykov and others added 3 commits December 16, 2020 10:29

Update js/lib/build.sh

0280e20

Co-authored-by: Ivan Mirić <[email protected]>

Specify node version and force the install

5adb5ef

Change the test for core-js compatibility

affa942

imiric reviewed Dec 16, 2020

View reviewed changes

simskij approved these changes Jan 5, 2021

View reviewed changes

simskij reviewed Jan 5, 2021

View reviewed changes

js/lib/k6-shim.js Show resolved Hide resolved

This was referenced Jan 8, 2021

Implement Array.prototype.flat(Map) with some basic tests dop251/goja#240

Merged

Update goja to latest master #1782

Merged

na-- modified the milestones: v0.30.0, v0.31.0 Jan 13, 2021

na-- mentioned this pull request Jan 26, 2021

Update babel&corejs fix the process for updating js libraries #981

Closed

mstoykov closed this Jan 28, 2021

mstoykov deleted the onlySomeCoreJSVer3 branch February 3, 2021 12:42

mstoykov restored the onlySomeCoreJSVer3 branch February 3, 2021 12:42

mstoykov deleted the onlySomeCoreJSVer3 branch February 3, 2021 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop some amount of corejs also the file in git tree #1772

Drop some amount of corejs also the file in git tree #1772

mstoykov commented Dec 15, 2020

mstoykov commented Dec 15, 2020 •

edited

Loading

mstoykov commented Dec 15, 2020 •

edited

Loading

imiric Dec 15, 2020 •

edited

Loading

simskij Jan 5, 2021

imiric left a comment

imiric Dec 15, 2020

mstoykov Dec 16, 2020

simskij Jan 5, 2021

imiric commented Dec 15, 2020

mstoykov commented Dec 16, 2020

imiric Dec 16, 2020 •

edited

Loading

mstoykov Dec 16, 2020

imiric Dec 17, 2020

simskij left a comment

simskij commented Jan 5, 2021 •

edited

Loading

mstoykov commented Jan 28, 2021

	modules="$(cat shim.js \| grep ^require \| sed "s\|require('\./modules/\|\|g" \| sed "s/');//g" \| tr "\n" ",")"
	modules="$(sed -n "s:^require('\./modules/\(.*\)');:\1:p" shim.js \| paste -sd',')"

Drop some amount of corejs also the file in git tree #1772

Drop some amount of corejs also the file in git tree #1772

Conversation

mstoykov commented Dec 15, 2020

mstoykov commented Dec 15, 2020 • edited Loading

mstoykov commented Dec 15, 2020 • edited Loading

imiric Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

simskij Jan 5, 2021

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

imiric Dec 15, 2020

Choose a reason for hiding this comment

mstoykov Dec 16, 2020

Choose a reason for hiding this comment

simskij Jan 5, 2021

Choose a reason for hiding this comment

imiric commented Dec 15, 2020

mstoykov commented Dec 16, 2020

imiric Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

mstoykov Dec 16, 2020

Choose a reason for hiding this comment

imiric Dec 17, 2020

Choose a reason for hiding this comment

simskij left a comment

Choose a reason for hiding this comment

simskij commented Jan 5, 2021 • edited Loading

mstoykov commented Jan 28, 2021

mstoykov commented Dec 15, 2020 •

edited

Loading

mstoykov commented Dec 15, 2020 •

edited

Loading

imiric Dec 15, 2020 •

edited

Loading

imiric Dec 16, 2020 •

edited

Loading

simskij commented Jan 5, 2021 •

edited

Loading