Skip to content
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

Disable some babel plugins that are now part of goja or are not useful #1822

Merged
merged 1 commit into from
Feb 2, 2021

Conversation

mstoykov
Copy link
Contributor

Unfortunatelly babel doesn't let you disable plugins from a preset, so
we need to specify every plugin that we want with it corresponding
configuration.

@mstoykov mstoykov force-pushed the enableAlmostAllTc39Tests branch from 351500b to 928e730 Compare January 26, 2021 18:33
@mstoykov mstoykov force-pushed the chore/disableNotNeededBabel branch from 40282a5 to 37add28 Compare January 26, 2021 19:10
@mstoykov mstoykov marked this pull request as ready for review January 26, 2021 19:11
@mstoykov mstoykov requested review from imiric and na-- January 26, 2021 19:32

// es2017 https://github.com/babel/babel/blob/v6.26.0/packages/babel-preset-es2017/src/index.js
// "syntax-trailing-function-commas", // in goja
// "transform-async-to-generator", // Doesn't really work unless regeneratorRuntime is also added
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider including this along with transform-runtime then? It would add a lot of value to support generators and this poor-man's async, unless they would add a lot to the bundle and explode the memory requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we discussed this internally and we came to the conclusion that it is better to break all the async/await stuff that is halfway working and letting people do it on their own through the template-es6 or similar.

I removed the generator tests specifically because we were failing practically all of them because it was needing regeneratorRuntime. And I am heavily against adding regeneratorRuntime and reenabling plugins, possibly bringing back the polyfill for Promise. It will likely not work great, again all of those tools are not written for k6 but for nodejs/browsers. Goja does implement Ecmascript faithfully, but there are still possibilities that those tools require something special in one of the cases to work.

Testing this will likely show whether I am correct or not, but IMO "not async" async is not worth the time investment and explanation on why it is not working as someone would expect. And then the possibility of breaking changes once we actually start having real async.

imiric
imiric previously approved these changes Jan 27, 2021
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really dig into each enabled plugin and their configuration, but as long as the TC39 tests pass I think we can be confident that we're not breaking anything.

I like that they're now explicitly listed instead of being part of some obscure preset. Nice work! 👏

@mstoykov
Copy link
Contributor Author

I didn't really dig into each enabled plugin and their configuration,

The configuration is copied from the presets, which are linked above each section of plugins. So it should be exactly the same but without some of them.

I like that they're now explicitly listed

Yeah, this also means that in the future removing one will be just commenting a line and rerunning the tests. And let's hope we can remove all of them 🍾

@na-- na-- added this to the v0.31.0 milestone Jan 29, 2021
@mstoykov mstoykov force-pushed the enableAlmostAllTc39Tests branch 2 times, most recently from b71ee6d to 97689ae Compare February 1, 2021 16:39
Unfortunatelly babel doesn't let you disable plugins from a preset, so
we need to specify every plugin that we want with it corresponding
configuration.
@mstoykov mstoykov force-pushed the chore/disableNotNeededBabel branch from 37add28 to 805c987 Compare February 1, 2021 16:43
@codecov-io
Copy link

Codecov Report

Merging #1822 (805c987) into enableAlmostAllTc39Tests (97689ae) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@                     Coverage Diff                      @@
##           enableAlmostAllTc39Tests    #1822      +/-   ##
============================================================
- Coverage                     71.58%   71.54%   -0.04%     
============================================================
  Files                           181      180       -1     
  Lines                         13939    13938       -1     
============================================================
- Hits                           9978     9972       -6     
- Misses                         3328     3331       +3     
- Partials                        633      635       +2     
Flag Coverage Δ
ubuntu 71.54% <ø> (ø)
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/compiler/compiler.go 76.47% <ø> (ø)
js/common/initenv.go 86.66% <0.00%> (-13.34%) ⬇️
js/initcontext.go 90.10% <0.00%> (-2.20%) ⬇️
lib/executor/vu_handle.go 93.33% <0.00%> (-1.91%) ⬇️
cmd/ui_windows.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97689ae...805c987. Read the comment docs.

Base automatically changed from enableAlmostAllTc39Tests to master February 2, 2021 08:31
@mstoykov mstoykov dismissed imiric’s stale review February 2, 2021 08:31

The base branch was changed.

@mstoykov mstoykov requested a review from imiric February 2, 2021 08:31
@mstoykov mstoykov merged commit 5619259 into master Feb 2, 2021
@mstoykov mstoykov deleted the chore/disableNotNeededBabel branch February 2, 2021 08:37
@mstoykov mstoykov mentioned this pull request Feb 2, 2021
@mstoykov mstoykov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants