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

react-core npm module #627

Closed
wants to merge 11 commits into from
Closed

react-core npm module #627

wants to merge 11 commits into from

Conversation

petehunt
Copy link
Contributor

@petehunt petehunt commented Dec 2, 2013

This was challenging for three reasons:

  • Accessing internals is hard
  • __DEV__
  • You can't require() the browserified code via UMD (not a showstopper but annoying)

This solves the first problem by just stealing @zpao's #442. Now you can require('React/lib/EventPluginRegistry') and inject your own event plugins. I don't think we should document this until we shore up our API. However, I think this is enough to publish TapEventPlugin (perhaps as a separate npm module)

The second is solved by using envify which will do a syntax transform to substitute the NODE_ENV environment variable (see http://www.hacksparrow.com/running-express-js-in-production-mode.html). So this will transparently work and I tested that it stripped invariants and __DEV__ branches. Additionally I required the transform in package.json so they can just transparently use this package and not worry about __DEV__ at all!

The third is solved by introducing muffinizing! Hell yeah! I just replaced every instance of the word require in the browserified code with muffin. This is actually pretty much the only way this will work since many packaging systems look for calls to a function named require(). cc @epriestley

I suggest we follow this up by deprecating react-addons, moving each addon to its own npm package and having a "react-with-addons" npm package that just depends on the other stuff and can be browserified with wzrd.in.

Test plan

  • Standard test plans (grunt, examples etc)
  • Look at filesizes, they seem OK
  • Manually examine transformed sources, they have the right "production" === process.env.NODE_ENV
  • Manually examine browserified sources (.min.js and .js) with minification OFF, they have the right "production" === "a string" in there.
  • Disable simple header and minfication for .min.js, diff -u react.js react.min.js shows no changes in the files (this is the goal)
  • Create test app using renderComponentToString(), npm install ~/Projects/react/build/react-core/, then require('React') and require('React/lib/React'), both work in Node
  • With same test app, cp ~/Projects/react/build/react.js ./react-browserified.js, using require('./react-browserified.js') still works
  • Try the last two previous steps with browserify: browserify testapp.js > bundle.js, browserified bundle works in Node.
  • Manually inspect browserified bundle, see that process is shimmed
  • Try the last two previous steps with production mode browserify and minfication: NODE_ENV=production browserify testapp.js | uglifyjs -cm > bundle.min.js. File still runs and invariant() messages are nowhere to be found. File size is 33kb after gzip (good).

@petehunt
Copy link
Contributor Author

petehunt commented Dec 2, 2013

Solves #436 as well

@petehunt petehunt mentioned this pull request Dec 2, 2013
@epriestley
Copy link

Muffinization is a robust software pattern suitable for tackling many difficult challenges.

// allow browserified libraries to work with requirejs and
// other pacakgers that get confused by these calls.
function muffinize(src) {
return src.replace(/require/g, 'muffin');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels dangerous enough to me that it may be worth using a real JS parser (or lexer) since you don't want to change the word "require" if it appears in a string or as an object key.

'DOMProperty: Cannot use muffin using both attribute and property: %s'

Copy link
Member

Choose a reason for hiding this comment

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

Yup, there's no way I'll accept something that doesn't do this the right way. "Unless muffined by applicable law …"

@andreypopp
Copy link
Contributor

Is it really a common scenario to load browserified bundles with AMD loaders like requirejs?

  • If you use browserify and need async loading of bundles you would probably use some basic loader, AMD is overhead there.
  • If you use AMD then you probably use requirejs and requirejs can load CommonJS packages while in dev and so concatenate them for prod.
  • if you use AMD with loader other than requirejs then maybe you would use some conversion tool from CommonJS to AMD.

@andreypopp
Copy link
Contributor

Is it really a common scenario to load browserified bundles with AMD loaders like requirejs?

If I'm wrong and it is... then I think muffinization should be added to umd package which is used by browserify.

@petehunt
Copy link
Contributor Author

petehunt commented Dec 2, 2013

OK, I'll take out the muffinizer :(

@petehunt
Copy link
Contributor Author

petehunt commented Dec 4, 2013

suggested muffinizing in ForbesLindesay/umd#10

@zpao
Copy link
Member

zpao commented Dec 6, 2013

Few things:

  1. please update the version in the core package.json
  2. update the version in React.js
  3. we'll want to update the commonerConfig version in package.json (to force a cachebuster)
  4. the browser tests don't run - ReferenceError: process is not defined (cc @benjamn, @subtleGradient)

@petehunt
Copy link
Contributor Author

petehunt commented Dec 6, 2013

Will make these changes, but aren't package.json and React.js automatically updated to the latest version?

@zpao
Copy link
Member

zpao commented Dec 6, 2013

No, only docs/_config.yml is auto updated. The rest is manual (but that's why there are tests to make sure they are updated, they were failing 😉)

@petehunt
Copy link
Contributor Author

petehunt commented Dec 6, 2013

build is green now. i thought tests were broken in master, looks like i was wrong!

@andreypopp
Copy link
Contributor

first time browserifying is somehow slow for me — around 3500ms, probably due to envify :-(

@andreypopp
Copy link
Contributor

ignore my previous comment, I included both react and react-tools by mistake

@zpao
Copy link
Member

zpao commented Dec 6, 2013

Pulled locally, added a few more things, then screwed up with a fast-forward merge. 9270d3d...153b75f has all the commits.

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

Successfully merging this pull request may close these issues.

5 participants