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

Stop people from patching babylon by building it #3204

Merged
merged 1 commit into from
Dec 24, 2015
Merged

Conversation

sebmck
Copy link
Contributor

@sebmck sebmck commented Dec 23, 2015

Stops people doing evil stuff that we don't want to support or endorse.

@@ -0,0 +1,2 @@
require("./_util").updateMain("index.js");
require("child_process").execSync(__dirname + "/../../../node_modules/.bin/browserify -s babylon -e " + __dirname + "/../lib/index.js -o " + __dirname + "/../index.js", { encoding: "utf8" });
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 don't need no stinkin node API.

Copy link
Contributor

Choose a reason for hiding this comment

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

path.join here ... because windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok mum

Copy link
Contributor

Choose a reason for hiding this comment

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

👶 🍼

@codecov-io
Copy link

Current coverage is 84.81%

Merging #3204 into master will decrease coverage by -0.28% as of 6fe210d

@@            master   #3204   diff @@
======================================
  Files          215     215       
  Stmts        15617   15617       
  Branches      3341    3341       
  Methods          0       0       
======================================
- Hit          13289   13245    -44
- Partial        682     725    +43
- Missed        1646    1647     +1

Review entire Coverage Diff as of 6fe210d

Powered by Codecov. Updated on successful CI builds.

@amasad
Copy link
Member

amasad commented Dec 23, 2015

what is an example of it being patched?

@amasad
Copy link
Member

amasad commented Dec 23, 2015

heh, ok. Can you add a comment somewhere explaining what's happening?
Afterwards feel free to merge. This needs to be a minor version bump?

@sebmck
Copy link
Contributor Author

sebmck commented Dec 23, 2015

Sure. I'll leave this up for a day or two as it's not super urgent if anyone else wants to leave feedback. This should also make slightly faster since we wont be doing as much disk IO. Depending on how good this is we could consider doing it to more packages.

@forivall
Copy link
Contributor

Can't we just be responsible users, allow this, but nudge those who do to provide sufficient warning that they are doing this, and that users should expect no support from babel from this use?

@sebmck
Copy link
Contributor Author

sebmck commented Dec 24, 2015

What they're doing is irresponsible and the fact that it's possible does not reflect well on Babel.

@forivall
Copy link
Contributor

I think it reflects badly on babel that this change, imo, breaks the open/closed principle, by completely closing down the parser to extension. Yes, this is a hack, yes, it is irresponsible, but it is done because there isn't a well defined mechanism to extend the parser, which is a commonly desired use case.

@sebmck
Copy link
Contributor Author

sebmck commented Dec 24, 2015

This isn't something that is supported, this has been very explicit and clear. I'm sorry you feel that way.

@@ -0,0 +1,2 @@
require("./_util").updateMain("lib/index.js");
require("fs").unlinkSync(__dirname + "/../index.js");
Copy link
Contributor

Choose a reason for hiding this comment

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

path.joinnnnnn

@forivall
Copy link
Contributor

I know it's never been supported. However, I do hope that there could be something added to extend the parser in the future.

Moreover, I don't completely understand why you think that this reflects badly on babel. Are there any pending bug reports resulting from people using these plugins? etc?

@jamiebuilds
Copy link
Contributor

Supporting users extending the parser would be a huge burden on the project if done improperly. And if we do want to do it someday, we would want to do so with careful consideration.

We're well aware how popular the idea of extending the Babel parser is, which is exactly why we're shutting it down before anything popular gets built on it and we're forced to support it. Because the truth is people are not as responsible as you give them credit for, and at the end of the day they are only placing a burden on us.

sebmck added a commit that referenced this pull request Dec 24, 2015
Stop people from patching babylon by building it
@sebmck sebmck merged commit 97faab3 into master Dec 24, 2015
@sebmck sebmck deleted the no-babylon-patch branch December 24, 2015 04:13
@sebmck
Copy link
Contributor Author

sebmck commented Dec 24, 2015

Locking this as this isn't an appropriate place for this discussion and this change wont be reverted. Feel free to jump on the Babel Slack if you'd like to voice your concerns.

@babel babel locked and limited conversation to collaborators Dec 24, 2015
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.

None yet

6 participants