Skip to content

Automatically add all serverRegistration and clientRegistration files to webpack . #237

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

Closed
wants to merge 2 commits into from

Conversation

jDeppen
Copy link
Contributor

@jDeppen jDeppen commented Feb 26, 2016

One less step for the developer and encourages modularity of domains (as found in the README).

Review on Reviewable

@justin808
Copy link
Member

@jDeppen, Thanks! Interesting approach. I see the "Railsy Convention over Configuration" approach here.

I'm probably OK with this. I'd like to get some other reviewers to comment as well.

This can definitely make the generators a bit easier to build, @jbhatab @aaronvb as we don't have to add another directory every time.

Thanks again!

@alexfedoseev @josiasds Thoughts?


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from the review on Reviewable.io

@jDeppen
Copy link
Contributor Author

jDeppen commented Feb 27, 2016

Another option is to just look for clientRegistration.js* and serverRegistration.js* files rather than hard-coding the rest.

Seems like it would be nice to add/remove folders in "bundles" and limit outside configuration as much as possible.

Thanks.


Review status: 2 of 4 files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@justin808
Copy link
Member

@jDeppen @jbhatab We might be changing to a pattern of just using one registration.jsx file if server and client are the same (not for the ReactRouter examples, but the basic app generator in React on Rails). We might change the glob to match that. Stay tuned.

app: [
'./app/bundles/comments/startup/clientRegistration',
],
app: glob.sync('./app/bundles/**/startup/clientRegistration.js*'),
Copy link
Member

Choose a reason for hiding this comment

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

@alexfedoseev @robwise Too much magic, or something we'd use?

Copy link
Member

Choose a reason for hiding this comment

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

It's ok if we want to compile all bundles into single output, but usually it looks like this:

someBundle: './app/bundles/someBundle/startup/clientRegistration.js',
someOtherBundle: './app/bundles/someOtherBundle/startup/clientRegistration.js',

So globbing doesn't help much.

@justin808
Copy link
Member

@jDeppen I spoke with Alex about this.

This is probably not a best practice because many apps will want to split up the bundles. The advantage of having separate bundles is that you can change one bundle, and the others can stay cached in the browser.

that is why we separate the vendor-bundle and the app-bundle

That being said, I think for a smaller app, you won't have many bundles, and it's just as well to manually list multiple registration files if you have that.

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.

3 participants