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

Drop corejs #1824

Merged
merged 1 commit into from
Feb 2, 2021
Merged

Drop corejs #1824

merged 1 commit into from
Feb 2, 2021

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Jan 26, 2021

Everything but:

  • Promises that aren't really working
  • define/lookup getter/setter which are AnnexB
  • other stuff that aren't standard
    is now in goja, so there is no need for corejs

Things that will break:

The one thing that now going over this shim.js from the previous PR, which might give us trouble is that flatten was renamed to flat somewhere along the proposal process. But corejs had it with the old name as that happened after that version. I think we shouldn't do anything and see if this actually breaks for users as this is 1 line change and will align with what is in browsers for example where flatten isn't defined.

@mstoykov mstoykov requested review from na-- and imiric and removed request for na-- January 26, 2021 19:31
@codecov-io
Copy link

Codecov Report

Merging #1824 (5edd8d5) into chore/disableNotNeededBabel (37add28) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           chore/disableNotNeededBabel    #1824      +/-   ##
===============================================================
- Coverage                        71.58%   71.57%   -0.02%     
===============================================================
  Files                              181      179       -2     
  Lines                            13939    13905      -34     
===============================================================
- Hits                              9978     9952      -26     
+ Misses                            3328     3323       -5     
+ Partials                           633      630       -3     
Flag Coverage Δ
ubuntu 71.54% <ø> (-0.02%) ⬇️
windows 70.06% <ø> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
js/bundle.go 91.83% <ø> (+1.17%) ⬆️
lib/executor/vu_handle.go 95.23% <0.00%> (+1.90%) ⬆️

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 37add28...5edd8d5. Read the comment docs.

@na-- na-- added this to the v0.31.0 milestone Jan 27, 2021
@na-- na-- mentioned this pull request 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.

This is YUGE! 👏

Did you run some benchmarks before/after? It would be good to know the difference and mention it in the release notes.

@mstoykov
Copy link
Contributor Author

Did you run some benchmarks before/after

I didn't have time for that as well ...

@mstoykov mstoykov force-pushed the chore/disableNotNeededBabel branch from 37add28 to 805c987 Compare February 1, 2021 16:43
Base automatically changed from chore/disableNotNeededBabel to master February 2, 2021 08:37
Everything but:
- Promises that aren't really working
- define/lookup getter/setter which are AnnexB
- other stuff that aren't standard
is now in goja, so there is no need for corejs
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'm OK with this, just two questions:

  • There are a lot of new errors in breaking_test_errors.json. Are we OK with them? I can't really tell whether they are important or not.

  • Maybe for a separate PR, but could you document how breaking_test_errors.json is generated exactly? I followed the js/tc39/README.md, emptied it with {} and ran the tests, but no JSON was generated, though I didn't wait for it to finish. Do you parse the out.log and convert it to JSON? Exact commands would be helpful here.

@mstoykov
Copy link
Contributor Author

mstoykov commented Feb 2, 2021

  • all the new errors are define/lookup-getter/setters, which we decided to dro, and some Promise ones as again the tc39 tests aren't the best-tagged tests ever :(

  • It is in the output. I run it with -v (not certain it's required 🤔 ) and write it to a file. Then I open it and search for FAIL - the JSON will be just before it. It takes 2-3 seconds to delete the file to only contain the JSON in vim and then I move it on the breaking_test_errors.json.

Ideally, this will be automated (somewhat) one day, but currently, if you don't clean the file it will not work properly and I don't want to spend an afternoon fixing that, given that we will likely stop touching this that much after a few more changes. If anything I am more interested in optimizing it to be kind of "faster" but that will take longer and given that it will naturally become faster, the less babel is needed, I am more for trying to

@na-- na-- added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Feb 2, 2021
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM. Added a breaking change tag, since we probably should at least mention this as a minor breaking change, despite the fact nobody will probably notice it. Please update the PR description with what you think the note in the release notes should be. Ideally now, while everything is still fresh in your mind.

@mstoykov
Copy link
Contributor Author

mstoykov commented Feb 2, 2021

@na-- I've updated the description

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