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

Remove check-constants plugin #6987

Merged
merged 2 commits into from Jan 26, 2018
Merged

Conversation

@maurobringolf
Copy link
Contributor

maurobringolf commented Dec 7, 2017

Q                       A
Fixed Issues? Fixes #5967
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? no
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

This is a recreation of #6661 including the recent changes to check-constants.

As discussed in the issue, I moved the code from check-constants into block-scoping including the tests. I also removed all references to the plugin I could find.

I had to pass state into the BlockScoping class to access addHelper. I am not sure if this is the best way to do it, but it seems to work so far.

@maurobringolf maurobringolf changed the title Rebased onto new version Remove check-constants plugin Dec 7, 2017
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Dec 7, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6692/

Copy link
Member

nicolo-ribaudo left a comment

Can you put the new code in a checkConstants method instead of updateScopeInfo?

Mauro Bringolf
@maurobringolf maurobringolf force-pushed the maurobringolf:move-check-constants branch from 4ce5d08 to 98fa9ab Jan 23, 2018
@maurobringolf

This comment has been minimized.

Copy link
Contributor Author

maurobringolf commented Jan 23, 2018

@nicolo-ribaudo Sorry for the delay. I'd still like to remove the check-constants plugin so I rebased the PR. The CI passed (98fa9ab) but after I moved the constants check into a separate method it failed due to timeout. Locally it passes.

Any ideas? Does this way trigger redundant constant check runs through scopes and make things slower than before? I thought the DONE set prevents that.

@existentialism

This comment has been minimized.

Copy link
Member

existentialism commented Jan 23, 2018

@maurobringolf unlikely related to your PR/changes, restarted the build on circle

@maurobringolf

This comment has been minimized.

Copy link
Contributor Author

maurobringolf commented Jan 23, 2018

Ahh nice, thank you! I didnt ask because the last time I did, it actually was an error in my code 😁 .

@xtuc
xtuc approved these changes Jan 26, 2018
@@ -1,58 +0,0 @@
# @babel/plugin-check-constants

> Validate ES2015 constants (prevents reassignment of const variables).

This comment has been minimized.

Copy link
@hzoo

hzoo Jan 26, 2018

Member

next pr: we should maybe include parts of this readme into the blocking scoping one?

This comment has been minimized.

Copy link
@maurobringolf

maurobringolf Jan 26, 2018

Author Contributor

Good point, I can do this later tonight (today for you 😁)

This comment has been minimized.

Copy link
@maurobringolf

maurobringolf Jan 26, 2018

Author Contributor

@hzoo I just added a note for the constants check in the readme #7279. Improvements to wording welcome! Anything else that is needed from a documentation perspective?

@hzoo hzoo merged commit 92fc26d into babel:master Jan 26, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.07% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maurobringolf maurobringolf deleted the maurobringolf:move-check-constants branch Jan 28, 2018
benjamn added a commit to meteor/babel-preset-meteor that referenced this pull request Feb 13, 2018
aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
* Rebased onto new version

* Moved constants check into a separate method
@lock lock bot added the outdated label May 2, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.