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

Fixes #5108, browser.js and browser.js test removed #5124

Merged
merged 8 commits into from Jan 20, 2017

Conversation

@Pr0x1m4
Contributor

Pr0x1m4 commented Jan 15, 2017

Q A
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? no
Deprecations? yes
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets Fixes #5108
License MIT
Doc PR no
Dependency Changes no

The files https://github.com/babel/babel/blob/master/packages/babel-core/src/api/browser.js and https://github.com/babel/babel/blob/master/packages/babel-core/test/_browser.js were not being used, thus this PR removes them.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jan 15, 2017

@Pr0x1m4, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo and @josh to be potential reviewers.

@Pr0x1m4, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo and @josh to be potential reviewers.

@hzoo hzoo added this to the Babel 7 milestone Jan 15, 2017

@hzoo

hzoo approved these changes Jan 15, 2017

Yep lgtm, nice stuff 😄

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jan 15, 2017

Member

also cc @Daniel15

Member

hzoo commented Jan 15, 2017

also cc @Daniel15

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jan 15, 2017

Current coverage is 89.62% (diff: 100%)

No coverage report found for master at fabfc46.

Powered by Codecov. Last update fabfc46...23df61d

codecov-io commented Jan 15, 2017

Current coverage is 89.62% (diff: 100%)

No coverage report found for master at fabfc46.

Powered by Codecov. Last update fabfc46...23df61d

@@ -1,112 +0,0 @@
-/* eslint max-len: 0 */

This comment has been minimized.

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

There is also a reference to this file in package.json that we should delete now.

It'd also be great to move api/node.js to be index.js now that we don't have the split entry point.

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

There is also a reference to this file in package.json that we should delete now.

It'd also be great to move api/node.js to be index.js now that we don't have the split entry point.

This comment has been minimized.

@Pr0x1m4

Pr0x1m4 Jan 15, 2017

Contributor

I've checked package.json but I haven't seen any mention of browser.js. Am I missing something or possibly looking in the wrong package.json ?

@Pr0x1m4

Pr0x1m4 Jan 15, 2017

Contributor

I've checked package.json but I haven't seen any mention of browser.js. Am I missing something or possibly looking in the wrong package.json ?

This comment has been minimized.

@Pr0x1m4

Pr0x1m4 Jan 15, 2017

Contributor

Also could you elaborate on moving api/node.js to index.js? I tried a refactor/rename in associated files and unit tests but it lead to breaking build.

@Pr0x1m4

Pr0x1m4 Jan 15, 2017

Contributor

Also could you elaborate on moving api/node.js to index.js? I tried a refactor/rename in associated files and unit tests but it lead to breaking build.

This comment has been minimized.

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

Oh! I was totally wrong about the package.json point I made, sorry. I guess the code I was thinking of got removed a while ago.

Yeah, my proposal was to move api/node.js to index.js since that is one of the few places that loads it. I'd expect you to have to update a couple other references to load the index instead, but that seems fine?

@loganfsmyth

loganfsmyth Jan 15, 2017

Member

Oh! I was totally wrong about the package.json point I made, sorry. I guess the code I was thinking of got removed a while ago.

Yeah, my proposal was to move api/node.js to index.js since that is one of the few places that loads it. I'd expect you to have to update a couple other references to load the index instead, but that seems fine?

This comment has been minimized.

@Pr0x1m4

Pr0x1m4 Jan 16, 2017

Contributor

No problem and thanks for explaining 😄

@Pr0x1m4

Pr0x1m4 Jan 16, 2017

Contributor

No problem and thanks for explaining 😄

packages/babel-core/src/index.js
+export { default as resolvePreset } from "./helpers/resolve-preset";
+export { version } from "../package";
+
+import * as util from "util";

This comment has been minimized.

@loganfsmyth

loganfsmyth Jan 16, 2017

Member

Looks like this is missing the ./

@loganfsmyth

loganfsmyth Jan 16, 2017

Member

Looks like this is missing the ./

This comment has been minimized.

@Pr0x1m4

Pr0x1m4 Jan 16, 2017

Contributor

Ahh, thanks for poiting this out, fixed.

@Pr0x1m4

Pr0x1m4 Jan 16, 2017

Contributor

Ahh, thanks for poiting this out, fixed.

This comment has been minimized.

@existentialism

existentialism Jan 16, 2017

Member

Also, not sure the "move" took hold, as it looks like api/node.js still exists?

Doesn't look like we need the api folder anymore either.

@existentialism

existentialism Jan 16, 2017

Member

Also, not sure the "move" took hold, as it looks like api/node.js still exists?

Doesn't look like we need the api folder anymore either.

@hzoo hzoo changed the base branch from master to 7.0 Jan 20, 2017

@hzoo hzoo merged commit 1742035 into babel:7.0 Jan 20, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@hzoo hzoo referenced this pull request Jan 20, 2017

Closed

[7.0] Consider removing browser.js from babel-core #5108

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment