Added jsx-self babel transform plugin #3540

Merged
merged 1 commit into from Jun 22, 2016

Projects

None yet

7 participants

@jimfb
jimfb commented Jun 20, 2016

This adds a new transform which appends the __self prop to jsx elements. The transform is needed by one of the React warnings that is being introduced, so users should enable it when running React in DEV mode.

@jimfb
jimfb commented Jun 20, 2016

cc @hzoo @kittens We should probably enable this by default (I think it's safe), but only in DEV mode. Do I need to add it to some presets file, or how does that work?

@jimfb
jimfb commented Jun 20, 2016

also cc @sebmarkbage

@hzoo
Member
hzoo commented Jun 20, 2016 edited

If you mean default to the react-preset yeah, actually already some code there that was commented out (due to http://phabricator.babeljs.io/T2994) doing a similar thing

https://github.com/babel/babel/blob/master/packages/babel-preset-react/index.js

@hzoo hzoo added this to the Next Minor milestone Jun 20, 2016
@codecov-io
codecov-io commented Jun 20, 2016 edited

Current coverage is 88.04%

Merging #3540 into master will decrease coverage by 0.06%

@@             master      #3540   diff @@
==========================================
  Files           193        194     +1   
  Lines         10072       9746   -326   
  Methods        1553       1510    -43   
  Messages          0          0          
  Branches       2199       2106    -93   
==========================================
- Hits           8875       8581   -294   
+ Misses         1197       1165    -32   
  Partials          0          0          

Powered by Codecov. Last updated by d0edc1c...88495bf

@hzoo
Member
hzoo commented Jun 20, 2016

@jimfb Also if you want, you can just push commits and we can squash into 1 commit at the end.

@jimfb jimfb referenced this pull request in facebook/react Jun 20, 2016
Open

Warn for string refs where owner != __self #7091

@jimfb
jimfb commented Jun 21, 2016

@hzoo The CI appears to be failing with Error: Cannot find module 'babel-plugin-transform-react-jsx-self'. Is this normal / to be expected for new modules, or is there still an issue on my end? Anything else I need to do on my end?

@hzoo
Member
hzoo commented Jun 21, 2016 edited

I see, it's because those tests are using the preset which now has the new module and I don't think lerna 1.x accounts for that. Not sure we can do anything other than switching to lerna 2.x which (I think) fixes it (#3509) or of course releasing/publishing it

@jimfb
jimfb commented Jun 21, 2016

@hzoo @kittens Ok, I'll assume the ball is in your court unless I hear otherwise.

@hzoo hzoo commented on the diff Jun 22, 2016
...es/babel-plugin-transform-react-jsx-self/src/index.js
+ *
+ * <sometag />
+ *
+ * becomes:
+ *
+ * <sometag __self={this} />
+ */
+
+const TRACE_ID = "__self";
+
+export default function ({ types: t }) {
+ let visitor = {
+ JSXOpeningElement(node) {
+ const id = t.jSXIdentifier(TRACE_ID);
+ const trace = t.identifier("this");
+ node.container.openingElement.attributes.push(t.jSXAttribute(id, t.jSXExpressionContainer(trace)));
@hzoo
hzoo Jun 22, 2016 Member

Also is it ok that there's no check for an existing __self attribute?

@kittens
kittens Jun 22, 2016 Member

Should be. The JSX transform will just output a object with multiple __self properties which is fine.

@hzoo
Member
hzoo commented Jun 22, 2016

@jimfb Ohh.. I checked the PR out locally and was still confused - it's because we need to add the plugin to the package.json not just the babelrc file haha.

@hzoo hzoo commented on an outdated diff Jun 22, 2016
...es/babel-plugin-transform-react-jsx-self/package.json
@@ -0,0 +1,18 @@
+{
+ "name": "babel-plugin-transform-react-jsx-self",
+ "version": "6.0.14",
@hzoo
hzoo Jun 22, 2016 Member

Not sure if this matters but I would change this to be the current version so 6.0.14

jim Added jsx-self babel transform plugin
7d0c4ec
@kittens kittens merged commit 9229225 into babel:master Jun 22, 2016

0 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@hzoo
Member
hzoo commented Jun 22, 2016

Added in 93299d9

@spicyj
Contributor
spicyj commented Jun 27, 2016

Didn't we decide last time against including dev-specific stuff in the dev environment for the preset because most people don't set BABEL_ENV (or NODE_ENV) properly during build time?

@sebmarkbage

Yea, this should not go into any preset that the source transform isn't in.

@hzoo
Member
hzoo commented Jun 27, 2016 edited

Ah sorry should of checked with someone else? I don't remember if we decided on that but I found http://phabricator.babeljs.io/T2994 which was regarding purerendermixin (although not sure how relevant since it's only in dev? Hmm it must be because it defaults to dev and like mentioned people don't set the env to prod.

Made #3552 to remove it from the preset

@SpiritBreaker226 SpiritBreaker226 added a commit to frig-js/frigging-bootstrap that referenced this pull request Jun 27, 2016
@greenkeeperio-bot @SpiritBreaker226 greenkeeperio-bot + SpiritBreaker226 Update babel-preset-react To Version 6.11.0 (#94) 18bd951
@jmm
Member
jmm commented Jun 27, 2016

@hzoo decided is a strong word, but there was some discussion about it (not between maintainers) starting here: #2963 (comment). I'm not sure I'm convinced that people are going to be oblivious to setting NODE_ENV, as often they already need to do that for other things, like bundling modules properly (React in particular), running server frameworks properly, etc. Not sure it's a big deal to need to configure this stuff explicitly rather than via the preset either, but it'll need good documentation somewhere for people to be aware of it.

@jimfb
jimfb commented Jun 27, 2016

Didn't we decide last time against including dev-specific stuff in the dev environment for the preset because most people don't set BABEL_ENV (or NODE_ENV) properly during build time?

I don't remember that being the decision. We backed out the __source transform from the default dev ENV because it broke the older versions of React which didn't have __source whitelisted yet. This change doesn't have the same concern since __self was whitelisted a long time ago, so it is "safer". We could probably also turn on the __source transform at this point. Anyway, that was my recollection, but maybe I'm mistaken, it was a long time ago.

@spicyj
Contributor
spicyj commented Jun 27, 2016

like bundling modules properly (React in particular), running server frameworks properly

I agree that people probably usually set NODE_ENV properly for runtime, but the question here is what NODE_ENV is when Babel is run, and I don't think most people set that. I know I usually don't.

@jmm
Member
jmm commented Jun 28, 2016

I'm not sure about the details of why jsx-source was removed from the preset. I noticed that it was blowing up when using the API to compile without the filename option, but that was getting fixed or got fixed. To the best of my recollection I saw a comment along the lines of "it's causing a number of problems" and I thought it was described as being removed until problems could be resolved, and I was under the impression that it being commented out was a reflection of that. I'd have to track down the issues / commits to be sure though.

but the question here is what NODE_ENV is when Babel is run, and I don't think most people set that. I know I usually don't.

I don't know if there's any way to get a meaningful amount of information about this kind of usage, but I would guess that a lot of people are compiling with Babel as part of bundling with Browserify or Webpack and setting NODE_ENV. How is this fundamentally different from needing to know that you need to set NODE_ENV when bundling React (for example)? Because if you bundle React for the wrong environment you might notice it via console warnings whereas this would be silent?

Can you help me out with some context and summarize how you're utilizing Babel when you're not setting NODE_ENV? E.g. CLI, API, Browserify + Babelify, Webpack + babel-loader, gulp-babel, some other plugin.

If we need to we can do more in the docs about guiding people to specify the environment.

@spicyj
Contributor
spicyj commented Jun 28, 2016

In webpack, you would typically use new DefinePlugin({'process.env': {NODE_ENV: JSON.stringify('production')}}), not set NODE_ENV in the build environment. Right? I guess if you are using envify then it's more likely.

@jmm
Member
jmm commented Jun 30, 2016

@spicyj Well, I realize there are a lot of work flows and tools I don't know about, that's why I wanted to better understand your scenario. I often use Babel with Browserify via Babelify, doing something like this for example:

./some-build-script --watch
# or
NODE_ENV=production ./some-build-script

If I'm bundling React with Browserify, envify (loose-envify) is running under the covers automatically and I'm setting the env for it as above.

If a user is just compiling some stuff with Babel and they don't have custom per-env configuration setup, I could see how they'd overlook setting the env or not realize it'd make a difference in that case (which maybe it hasn't up til now). Like I mentioned, if it is important (e.g. if stuff in Babel core is going to behave differently per-env) we could do more with the docs to educate people about that.

In webpack, you would typically use new DefinePlugin({'process.env': {NODE_ENV: JSON.stringify('production')}}), not set NODE_ENV in the build environment. Right?

The way that's illustrated makes it seem static -- in that scenario doesn't the user still need to dynamically set the NODE_ENV passed to Webpack somewhere from the shell or JS?

I don't know if your question was rhetorical, but if we want more perspective on Webpack usage I think @loganfsmyth, for one, knows way more about it than I do. Or I could ask on Slack -- I think a lot of Babel users use Webpack.

Here's one example I found (Redux) where they're doing this (interestingly setting both BABEL_ENV and NODE_ENV, to different values):

cross-env BABEL_ENV=commonjs NODE_ENV=development webpack src/index.js dist/redux.js

And passing that value to Webpack:

var env = process.env.NODE_ENV
// [...]
    new webpack.DefinePlugin({
      'process.env.NODE_ENV': JSON.stringify(env)
    })

(Granted, people working on Redux have sophisticated development skills.)

@hzoo hzoo modified the milestone: Next Minor Jul 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment