adds ability to require components already wrapped in factories #536
Conversation
|
I have to voice here that I'm not a fan of this feature, nor do I really care to maintain it. I feel that this is something that downstream users can apply on their own, or possibly maintain a separate project which does this wrapping. This is not the standard use case for the majority of those using React, and is really a hack to get around previous functionality that is no longer supported by react. |
|
I agree with @mtscout6. |
|
I agree with your point on hardcoding, and, and I will look for a better solution if you're amenable to merging it. Otherwise I will just continue re-creating the factories each time as the consumer. After reading the notes on this subject from the React team, I'm not sure a mixin is the way to go... but could be misunderstanding. I would point out that React.js does include these factories out of the box. It would seem to me that it would be ideal if react-bootstrap provided the same level of abstraction as the core library. If React didn't do this, I wouldn't have a leg to stand on though (and perhaps still don't). |
|
I saw those notes a few months ago, and I was under the impression that react was deprecating this. I misunderstood. I'm seeing the exact thing you are talking about at this line: https://github.com/facebook/react/blob/master/src/browser/ReactDOM.js#L30. I'm still not a fan, but I can see your point. I guess it would be ok to bring in after the previous suggestions along with some additional concerns are address. This is going to need some additional documentation so others are aware it's there and how to use it. If you could also please include caveats to using one or the other within that documentation as well so those that use it are aware of what it implies. I'm thinking the getting start page is as good a place as any to document it. This should also be tested, to ensure we don't break regression on it in the future. I'm thinking that it would probably make sense to not completely mirror the index.js api so you can easily iterate through all the factory keys and see if they render or not? I'm not entirely sure that's even the best way to test this. |
| let factories = {}; | ||
|
|
||
| Object.keys(components).forEach(function (name) { | ||
| if (components[name] instanceof React.Component.constructor) { |
There was a problem hiding this comment.
Does this work with components created with both React.createClass and ES6 style class NAME extends React.Component? To be safe I think it would be a good idea to extract this logic to a function and write some tests against it.
There was a problem hiding this comment.
Good point. I'm working on test cases and will be sure to check. However, there is a minor downside to this approach. Since there are no actual files, I cannot import like this:
var Alert = require('react-bootstrap/lib/factories/alert')I'm considering alternatives to make a recommendation. Do you have an opinion on adding files for the factories? It may be possible to generate them automatically in the build process...
var Alert = require('react-bootstrap/lib/alert-factory')There was a problem hiding this comment.
Ya I'd rather those be generated files.
There was a problem hiding this comment.
Those generated scripts can come in a follow up pull request or we can try and bundle with this one. You'll need to consider both a CommonJS and AMD generation 😉
There was a problem hiding this comment.
I'd be ok with a template file getting committed and used. We are already doing this for the bower.json file: https://github.com/react-bootstrap/react-bootstrap/blob/master/tools/amd/build.js#L18-L27
The template file would need a different extension than .js so babel doesn't try to transform it.
There was a problem hiding this comment.
Great, that gives me a nice way forward. I'll work on this and bundle everything together. Thanks!
|
Tried to generate the factory scripts during build, but the components in the tests are imported from src. To keep it consistent I generated files for each factory so they could be imported from src as well. Turns out to be the simplest approach anyway, and these files shouldn't have to change much if at all. Let me know if this works, or if you'd like to have the factory tests run from lib, in which case I can generate instead. Commit on the way shortly. |
|
I'd rather have the factories folder generated during the build. Otherwise maintaining new/moved/removed/renamed components will be overly tedious. There's no need to couple the factory tests with the existing component tests. You could write one test that generates a test for each file in the factories folder. We'd need to change the release process to build before running tests, and vice version when running |
|
OK, factories folder now generated during the build. Could probably use some touch up but wanted to see if you have any comments on the general approach first. |
| import components from './components'; | ||
|
|
||
| const repoRoot = path.resolve(__dirname, '../'); | ||
| const src = path.join(repoRoot, 'src'); |
There was a problem hiding this comment.
The repoRoot is now available through the tools/constants module, src would also be a good candidate to add to tools/constants if you want to.
|
It would be one more run of hits to disk, but you could generate the factory file directly to src/factories prior to invoking the lib and amd builds. That would keep both the lib and amd builds unmodified and easier to maintain since the babel build does work recursively on a given directory. That would also simplify the factories generation since it wouldn't be concerned about the babel transform. It would also allow you to inspect any given factory file prior to compilation should you need to. We can add an entry to the .gitignore for src/factories so it doesn't get committed. |
| function createTest(component) { | ||
| describe('factories', function () { | ||
| it(`Should have a ${component} factory`, function () { | ||
| let factory = require(`../lib/factories/${component}`); |
There was a problem hiding this comment.
If you wrote files to src/factories than this could change to the src folder which would be in line with the rest of the unit tests.
Also, you could have a before hook that writes missing factories to the src/factories folder. This is important for the npm run test-watch which runs tests in watch mode without a build.
|
Generating the factories in |
|
That's a good point. We could put a README in the factories folder which states that the folder is generated. However, that will still get overlooked by many people. 😕 My main thought for this was to support running the tests in watch mode. Currently the test-watch command's startup is fast which I'd like to keep. Which Is why I was thinking about some kind of src folder for these factories. I can see the flaws outweigh the gains, and I don't see any better way around it. I'm fine with striking the idea of Can you still address the other comments though? |
|
OK, for the moment I addressed all the points except test-watch. I'll look into test-watch and send another PR if I come up with a better way. I made the path constants changes in another PR #576. This PR now depends on that one, so if you would like you can merge that one first and then have me rebase before merging this one. |
|
Forgot one thing: Changed the constructor check function to a flat list of components because there were too many places that would need to read the components from disk and perform the check (CommonJS build, AMD build, and test generator all use the same list). It seemed a bit much. However I can re-add this if you would prefer it and don't mind the associated overhead. |
|
I don't mind the added overhead, since it's less to remember to maintain manually. |
|
It was a good call to build the component list dynamically. I found an error because I didn't realize that not all of the components are public (e.g. FormGroup). Anyway I re-added the automation to build the component list by checking the constructor, and DRYed up this code by adding it in a utility in |
|
I have written a couple of comments in the code https://github.com/tacomanator/react-bootstrap/commit/f6d9d85c388 😉 |
|
LGTM Need another sign-off by one of the @react-bootstrap/collaborators |
|
Code generated by As for the PR code - it is working. Tests are added. ✨ It seems, after merging, it will need to add a note about factories somewhere in the docs |
It would be awesome @tacomanator 👍 ❇️ ❇️ |
|
For reference
from here https://github.com/facebook/react/blob/master/CHANGELOG.md#deprecations-2 Would it be any value to reference this notice from React blog in the doc about factories ? |
|
@AlexKVal This pull request is resolving that deprecation, what exactly are you proposing? |
|
@tacomanator has written that he had updated I'm asking if there is any value to point to that deprecation notice in his article / note / topic about factories. Something like: |
|
Ah, ok that's what he's planning to put in the Getting Started page. |
|
I just did not see that page yet, hence my questions about it. |
|
OK, thanks for your patience guys. The documentation in back in now. |
|
LGTM nice job |
|
A long way has been walked. A great job has been done. |
[added] convenience factories for non-JSX users in lib/factories
|
Thank you all! |
Allows import of components pre-wrapped in factories with 'react-bootstrap/lib/factories'.
I had same problem as this guy: #423.