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

chore(allTheThings): update to universal-redux npm module (react-router 2, redux-simple-router, babel 6) #759

Closed
wants to merge 75 commits into from

Conversation

bdefore
Copy link
Collaborator

@bdefore bdefore commented Dec 31, 2015

The universal-redux npm package bumped to 3.0.0-beta1 with React Router 2 support. There are some regressions but expect to clean them up shortly: bdefore/universal-redux#24

This is a monster PR now, sorry about the huge diff. It's a descendant of #626 (npm package) #685 (redux-simple-router) and #730 (babel 6). I thought I would drop the latest here in case anyone else is considering updating to RR2 themselves; the diff at the issue above might be instructional for porting.

@quicksnap
Copy link
Collaborator

It's a descendant of #626 (npm package) #685 (redux-simple-router) and #730 (babel 6).

@bdefore May I close out those PRs? Should we also update the title of this one to include descriptions of all its goodies?

@bdefore
Copy link
Collaborator Author

bdefore commented Jan 7, 2016

@quicksnap sure, and i'll update that now.

@bdefore bdefore changed the title chore(routing): update to React Router 2 chore(routing): update to universal-redux npm module (react-router 2, redux-simple-router, babel 6) Jan 7, 2016
@bdefore bdefore changed the title chore(routing): update to universal-redux npm module (react-router 2, redux-simple-router, babel 6) chore(allTheThings): update to universal-redux npm module (react-router 2, redux-simple-router, babel 6) Jan 7, 2016
@andreijs
Copy link

Any chance of getting this merged in ?

@bdefore
Copy link
Collaborator Author

bdefore commented Jan 16, 2016

@andreijs at this point there's about a month of commits that would need to be brought in. also the api client's use of cookies would need to happen through a different manner, probably an express middleware change to put the cookie in the header.


var babelrc = fs.readFileSync('./.babelrc');
var babelrc = fs.readFileSync(path.resolve(__dirname, 'node_modules/universal-redux/.babelrc'));

Choose a reason for hiding this comment

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

Is this ok? relaying on a node_modules .babelrc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andresgutgon while it doesn't feel great to explicitly reach into node_modules, it allows universal-redux projects to work without having a project level .babelrc. ideally this server.babel.js file wouldn't be used at all, as it's only used to allow es6 files to be transpiled on the fly.

Choose a reason for hiding this comment

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

I don't know. It doesn't feel right to me. Could not be posible do it in another way?

What's the problem with having .babelrc here, in this project. And another .babelrc in universal-redux module?

Sorry, I just want to understand it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. The reason this file exists is so that this project can have ES6 in its server files and have transpilation on the fly. I think you're right that for the purpose of this project a .babelrc should live on project root, be configured in this file, and then be passed in through the universal-redux.config.js (via babelConfig) rather than reaching into node_modules, that way the internal concerns of universal-redux (such as react-transform-hmr configuration) don't leak into the project .babelrc configuration.

Choose a reason for hiding this comment

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

Thanks @bdefore
Again, great work 💪

@msikma
Copy link
Contributor

msikma commented Jan 17, 2016

Really great work! 👍

@msvalina
Copy link

Thanks for your effort!

@frankleng
Copy link

exciting!

@josmardias josmardias mentioned this pull request Feb 12, 2016
@bdefore
Copy link
Collaborator Author

bdefore commented Feb 16, 2016

A fresh PR and migration is probably in order in case anyone would like to use UR since merging in #833. Good news is that those changes bring the two projects closer together implementation wise. Bad news is that I'm not sure when I'll be able to get around to making a new PR.

@oyeanuj
Copy link

oyeanuj commented Feb 16, 2016

@bdefore looking forward to it whenever you get around to it! :)

@bdefore bdefore closed this Mar 16, 2021
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.

None yet

9 participants