[RFC] Convert source module format from ES6 to CommonJS

[pieterv]

The es6 module transpiler currently doesn't have a way to exclude 3rd party libs (and even if was added the grunt plugin is out of date anyway) which meant we have had to keep copies of React lib files converted to use es6 module imports in our repo, this is bad practice as it means we are unnecessarily locking react bootstrap to a specific react version and with React 0.11 on the near horizon not having this could make the transition between React version difficult as we can only support one version at a time. This change swaps the module format to CommonJS which (for the moment) has more mature tooling support allowing this functionality.

When swapping out the import statements i removed the additional whitespace aligning the ='s, this was becoming difficult to maintain and i didn't think worth the work to keep them in sync.

The tests are all passing but are throwing up the following error:

WARN: 'Warning: You are calling cloneWithProps() on a child with a ref. This is dangerous because you're creating a new child which will not be added as a ref to its parent.'

Since i haven't changed any functionality i assume we had just removed this error in our version of react/lib/cloneWithProps? But guess we will need to fix this properly if this change goes through.

The AMD files will need additional testing, i am just wrapping the CommonJS modules with define(function (require, exports, module) {}); via grunt-amd-wrap and it looks like it should run fine to me but i don't have a proper AMD project to test it in.

@stevoland how do you feel about this? I know we have only recently switched to es6 modules and it would be nice to transition back once the tools have stabilised.

[stevoland]

Yeah we should definitely do this. I've no time to check this out properly at the moment though - it will have to wait a week or so. Cheers.

[pieterv]

@stevoland Awesome, if its cool with you i will merge to master and create an issue to test the AMD modules.

[stevoland]

@pieterv I'd like to hold on the merge til I get a chance to check it out properly. We use AMD at work so I'd like to make sure it's right first. Cheers.

[pieterv]

Ok no problem :)

If anyone wants a React v0.11 compatible build (for CommonJS) just let me know and i can set one up until we can do a proper release.

[stevoland]

Had a quick look and the AMD build doesn't work as it doesn't include the React internal modules in the build.

[pieterv]

I assume you are talking about the amd/react-bootstrap.js bundled file? I have just pushed up a change for the requirejs grunt config which now includes all react internals needed. I have tested it locally so should work this time :P

[stevoland]

No, I was referring to the AMD modules themselves although it would affect the bundled file also. We need to include the required internal React modules themselves. When a user does require('react-bootstrap').Nav; that module does require('react/lib/cx'); which isn't defined anywhere but needs to be. I might be able to look into this at the weekend but my broken laptop's getting in the way.

[pieterv]

Right i understand now, i didn't realise the bower version of react didn't expose the internals as AMD modules. I assumed it was the same as the npm (commonjs) version. In this case i wonder whether we should switch to using react/addons and use these API's that way? I almost made this change the other day, but wasn't sure how it would work if something required react as well as react/addons, but looking at it now it should be fine. What do you think? Im happy to make that change and add it to this PR.

No good about your laptop, the screen died on mine (via me dropping it :|) a couple of weeks back so i feel your pain.

[tsing]

@pieterv Would you please release a React v0.11 compatible build (for CommonJS) ?

[pieterv]

@tsing i have created a build and pushed it to my github account, you should be able to install it via npm install pieterv/react-bootstrap-npm. Let us know if you have any problems with this.

[stevoland]

@tsing @pieterv Hmm, I thought RB 0.11 was compatable with React 0.11. Is it not?

[pieterv]

@stevoland it is in the sense that the tests pass and seems to work in general but since the react internals in RB are still from version 0.10 we don't know what edge cases could come up since we are doing things like cloneWithProps on react components from 0.11 but using 0.10 cloneWithProps logic. The build i did using this branch would be safer as it uses the internals from the react version you have installed. Does that make sense?

[tsing]

@pieterv The pieterv/react-bootstrap-npm works perfect in my project

[pieterv]

@tsing Awesome :)

@kellec is currently working on the change to using using react/addons instead of react/lib/*, which is looking pretty straight forward :)

[brigand]

If it's switched to browserify, you can generate a UMD with a simple config option, which gives you compatibility with commonjs (node or browserify), AMD, and window globals.

[stevoland]

@tsing @pieterv I'm not sure I'd like to require react.addons just for a couple of internal functions. In addition, we're using ReactTransitionEvents which isn't exposed in the addons. I'm comfortable using copies of these until the React module system makes it possible to access these modules properly.

@brigand Currently we support requiring individual modules require('react-boostrap').Button in AMD and I'd like to continue this. We would need each module to be wrapped in UMD and also a way to wrap these internal React modules.

I'm hoping to look into this this week.

[pieterv]

@stevoland see facebook/react#1906 on progress for React making internals available in some form.

Also agree about the react/addons after actually thinking about it how you would have to configure it. @kellec has some code pulling these react internals into react-bootstrap, the major one to copy is cloneWithProps, we will get that finished and push it up for review.

[kellec]

I've removed all references to react/lib files, and where we need access to an unexposed React method, the functionality has been copied into it's own file and placed into a /utils directory to try to keep the main src directory for components. Also split up the former utils.js file and removed the deprecated functions.

We ended up with classSet, cloneWithProps, EventListener, createChainedFunction, merge, and TransitionEvents. Since they're their own files we can easily remove them if/when the method becomes available (which may happen in a staggered fashion per the link from @pieterv above). Where relevant, the files contain comments to this effect. This way we don't have to require react/addons which was going to be a giant pain when supporting both AMD and CommonJS.

This is passing the tests and the docs site works fine, which takes care of the CJS modules. I set up a quick AMD testing environment to check it, and it's working fine there too.

[stevoland]

@kellec This sounds perfect! I'll try to check it out tonight.

[stevoland]

@kellec Sorry for the delay. This is perfect, thanks so much.

@pieterv I'm going to do a patch release for this. It seems there's no breaking changes, right?

[pieterv]

Awesome! :)

@stevoland no breaking changes to the public components, the only breaking change will be if someone was requiring one of our internal utils modules which have now moved.

[stevoland]

I've released 0.11.1 with all the new hotness - great work.

[kellec]

Yay, awesome :D

[pieterv]

Awesome! You got to love any pull request thats +710 but −17,994 lines :P