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

Split meteor-react mixins into a separate package? #11

Closed
grigio opened this issue May 24, 2015 · 6 comments
Closed

Split meteor-react mixins into a separate package? #11

grigio opened this issue May 24, 2015 · 6 comments

Comments

@grigio
Copy link
Contributor

grigio commented May 24, 2015

In this package you bundle react + meteor-react mixins, a problem I see with this approch is that if you want to use the rest of React ecosystem is problematic, because:

  • you need to upgrade grove:react at every new react release
  • you can't install other React packages e.g Material UI

An alternative (ultimate :) ) approch could be:

  • install React and React specific packages via: cosmos:browserify by @elidoran
  • install meteor-react mixins
  • .. start Meteor React coding

What do you think?

@lourd
Copy link
Member

lourd commented May 24, 2015

I like this idea! Agreed on the problem, I really dislike wrapper packages for other libraries. I'd seen him announce the browserify package on the forums but hadn't played with it till getting this message. Thanks for the encouragement 😄

So yeah, I'm down to remove the vendor files from this package. I'll remove the files, include some instructions in the README on using cosmos:browserify, and bump it to v0.2.

@lourd
Copy link
Member

lourd commented May 24, 2015

Just realized one semi-significant tradeoff in doing this. React's production version is more than just a minified version of development. There's some other optimizations that they do. That's why in the package.js we check if it's meteor build or not and include the production version if it is. I don't see anyway to do that with the browserify approach 😞

@grigio
Copy link
Contributor Author

grigio commented May 24, 2015

@elidoran I'm confused. If I use cosmos:browserify to install React and other React dependencies (e.g Material UI), shouldn't be cosmos:browserify to select the React minified version?
..and grove:react just add DDPMixin.js , ReactiveMixin.js ?

@elidoran
Copy link

I've been working on my next comment. I can see how my initial code example seems confusing as it's not a solution. It's the first thing I noticed about this project. It doesn't need to maintain the vendor files inside it, and, because the react module provides an already browserified file, it can get the file it wants from the npm module without performing a browserify. So, I showed how to do that.

I looked at material-ui. If you require it in a browserify.js file it'll use lib/index.js and give you all of it.

I looked at react. Its packages.json file has a browserify config to apply envify transform. Its main file is react.js, so, that's what would get browserified when targeting the module. It seems what's desired is browserifying the addons.js file instead. Add to that the consideration its pre-built production distribution is customized, it makes browserifying the module problematic.

cosmos:browserify wouldn't select a file to include. What it does, is have the npm browserify module process your browserify.js file with the .npm/package/ folder as the target to find the npm modules required. It lets Meteor minify the result when building for production. So, doing React = require('react'); causes it to browserify the react module based on its packages.json file. It doesn't choose between the files in the dist folder.

So, if you use the prebuilt file then it shouldn't be browserified because it already is. If you want to browserify it yourself with cosmos:browserify then it won't produce the same as the prebuilt file because it targets react.js not addons.js (also, if its production file is customized instead of simply being minified).

@lourd
Copy link
Member

lourd commented May 26, 2015

Hi @elidoran! Thanks for chiming in. Nice work on your package, clever workaround 👍

Here's my observations so far:

  • You can get the addons version without a problem. Putting this in browserify.js works:
React = require('react/addons');

And then you get React.addons. So that's good, and not a problem.

Unfortunately it doesn't look like envify is getting applied. I made a new Meteor app that only does this:

var Thing = React.createClass({
  render: function() {
    this.props.foo = "Look I'm mutating props";
    return (
      <h1>Hello Poppet</h1>
    );
  }
});

document.addEventListener("DOMContentLoaded", function(event) {
  React.render(<Thing/>, document.body);
});

When using the development version you get a warning not to mutate props. In production it should be silent. Normally you get the production version by setting NODE_ENV to "production", envify automatically picks that up when you browserify, fills in the spots where process.env.NODE_ENV is used in the code with normal strings, and then, when minifying, UglifyJS strips out the now dead code.

I'm looking at the source being delivered to my browser and I see process.env.NODE_ENV everywhere. Those aren't being replaced by envify. Therefore it's dead code, so it never gets stripped out when the Meteor build process runs Uglify for us.

I didn't fully understand how that worked before, envify is pretty cool. I will go ahead and take the vendor files out and put a little how-to in the README for doing this, but also include a note that until we figure out the envify thing the extra warnings will be included

@lourd
Copy link
Member

lourd commented May 26, 2015

Done in 5419f51. @elidoran, let me know if it'll be an issue getting envify included

@lourd lourd closed this as completed May 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants