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

Babel preset & cleanup #135

Merged
merged 11 commits into from
Apr 29, 2016
Merged

Babel preset & cleanup #135

merged 11 commits into from
Apr 29, 2016

Conversation

zpao
Copy link
Member

@zpao zpao commented Apr 13, 2016

This revives #127. @yungsters is out for a little while so not going to put this on him.

  • Make it so that .js.flow files can also be generated with the preset so project doesn't have to declare any plugins explicitly
  • Cleanup some of the dependencies (some were added or moved in the split process, needs a pass)
  • Make sure tests are working (Scroll is failing)
  • Remove the remaining Babel 5 stuff from fbjs-scripts (will likely just ship 0.7 after this is done with this being the only change).

yungsters and others added 8 commits April 21, 2016 22:02
@zpao
Copy link
Member Author

zpao commented Apr 22, 2016

Ok, rebased everything from the beginning so we shouldn't be introducing any unnecessary deps and un-rearranged the package.jsons for easier diffing (I know, npm did it but it was excruciating to rebase so hopefully I didn't screw that up).

Last but not least, here's the diff of lib/: https://gist.github.com/zpao/5e8d8b2badf781d6c638b42bc56fd862. As you can see the only difference is the .js.flow files for modules using Promise, they are now getting the autorequire transform running. Should probably change that back just to keep fbjs at status quo (it's a 1 line fix in the gulpfile)

@zpao
Copy link
Member Author

zpao commented Apr 29, 2016

Alright, calling this done. Will merge & ship babel-preset-fbjs@1.0.0 and fbjs-scripts@0.7.0 in a few minutes.

cc @kassens

@zpao zpao merged commit 1458e4e into facebook:master Apr 29, 2016
@zertosh
Copy link
Member

zertosh commented May 5, 2016

Is pulling out babel-plugin-syntax-flow too much of a breaking change to do a release of fbjs that's semver compatible with ^0.8.0 (i.e. react@15 compatible)?

Now that babel 6 is compiled with babel 6, the babel family of packages is using babel-runtime@6. In Nuclide we have a top-level babel-runtime@5 (we're stuck with babel@5 for the near-future – because Atom). fbjs, and few other packages we use, have undedupable babel-runtime@6s that nearly doubled our node_modules :( I'm trying to trim it where I can.

@zpao
Copy link
Member Author

zpao commented May 5, 2016

Are you talking about fbjs, fbjs-scripts, or babel-preset-fbjs? We aren't using babel-runtime in fbjs itself so presumably you're talking about the preset but you don't have to use that.

@zertosh
Copy link
Member

zertosh commented May 5, 2016

I'm talking about fbjs. You're forced to install babel-runtime via react@15.0.2 -> fbjs@0.8.1 -> babel-plugin-syntax-flow@6.8.0.

└─┬ react@15.0.2
  ├─┬ fbjs@0.8.1
  │ ├─┬ babel-plugin-syntax-flow@6.8.0
  │ │ └─┬ babel-runtime@6.6.1
  │ │   └── core-js@2.3.0
  │ ├── core-js@1.2.6
  ...

I know React doesn't need babel-plugin-syntax-flow or babel-runtime, that's why I'm humbly asking if dcc63b3 can be published as fbjs@0.8.2 (i.e. semver compat with React's dep on fbjs@^0.8.0). I don't know if that'll break any other fbjs consumers https://www.npmjs.com/browse/depended/fbjs.

@zpao
Copy link
Member Author

zpao commented May 5, 2016

oooohhh I see. I was confused because that's distinct from this PR :) That should never have been a dependency, I missed it in #121. It's definitely unused and we can publish fbjs@0.8.2 to fix.

ghost pushed a commit to facebookarchive/nuclide that referenced this pull request May 6, 2016
Summary: `fbjs` was updated to not include `babel-plugin-syntax-flow`, this reduces our production builds by ~7.5mb. Thanks zpao! See facebook/fbjs#135 (comment).

Reviewed By: ssorallen

Differential Revision: D3267846

fb-gh-sync-id: eebb23dd05ca72168a3c7beb717f10d3779a864e
fbshipit-source-id: eebb23dd05ca72168a3c7beb717f10d3779a864e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants