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

v9 Migration Doc #957

Merged
merged 4 commits into from
Oct 26, 2017
Merged

v9 Migration Doc #957

merged 4 commits into from
Oct 26, 2017

Conversation

Judahmeek
Copy link
Contributor

@Judahmeek Judahmeek commented Oct 16, 2017

This change is Reviewable

@justin808
Copy link
Member

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


docs/basics/upgrading-react-on-rails.md, line 9 at r1 (raw file):

* If you have `react_component` returning hashes, then switch to `react_component_hash` instead

## Upgrading to version 9

from v8?


docs/basics/upgrading-react-on-rails.md, line 20 at r1 (raw file):
I don't follow this one.

webpacker just makes it so you don't have to do your own webpack configuration

This enables your webpack bundles to bypass the Rails asset pipeline and it's extra minification

that's moving beyond v7 of RoR


docs/basics/upgrading-react-on-rails.md, line 23 at r1 (raw file):

#### From version 8
See [our changelog for instructions](https://github.com/shakacode/react_on_rails/blob/master/CHANGELOG.md#90-from-8x-upgrade-instructions)

let's include this here


docs/basics/upgrading-react-on-rails.md, line 32 at r1 (raw file):

*   `Gemfile`: bump `react_on_rails` and add `webpacker`
*   layout views: anything bundled by webpack will need to be requested by a `javascript_pack_tag` or `stylesheet_pack_tag`
*   `config/initializers/assets.rb`: Delete it. You don't need it anymore.

maybe revert back to default for assets.rb? I'm not sure about deleting it.


Comments from Reviewable

@justin808
Copy link
Member

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


Comments from Reviewable

@Judahmeek
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.


docs/basics/upgrading-react-on-rails.md, line 20 at r1 (raw file):
Webpacker does make it so that you don't have to do your own webpack configuration, but clients with legacy projects are likely to want to maintain control of their webpack configuration. Therefore, the only other reason a client would want to integrate webpacker is to bypass the double minification.

that's moving beyond v7 of RoR
Anything involving webpacker is moving beyond RoR v7. What's your point?


docs/basics/upgrading-react-on-rails.md, line 23 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

let's include this here

Will do.


docs/basics/upgrading-react-on-rails.md, line 32 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

maybe revert back to default for assets.rb? I'm not sure about deleting it.

Well, if we're going to tell people to revert it, then we'll have to identify exactly what changes RoR made to it. Shall we assume the client installed RoR v7?


Comments from Reviewable

@justin808
Copy link
Member

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions.


docs/basics/upgrading-react-on-rails.md, line 20 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Webpacker does make it so that you don't have to do your own webpack configuration, but clients with legacy projects are likely to want to maintain control of their webpack configuration. Therefore, the only other reason a client would want to integrate webpacker is to bypass the double minification.

that's moving beyond v7 of RoR
Anything involving webpacker is moving beyond RoR v7. What's your point?

This makes no sense at all to me.

Webpacker provides 2 things:

  1. View helpers that support bypassing the asset pipeline to enable things like avoiding double minification and proper source maps. This is 100% a best practice.
  2. A default Webpack config so that you only need to do minimal modifications and customizations. However, if you're doing server rendering, you may not want to give up control. Since Webpacker's default webpack config is changing often, we can give you definitive advice on this one. In general, if you're happy with doing your own Webpack configuration, then stick to that. Most bigger projects will prefer having more control.

docs/basics/upgrading-react-on-rails.md, line 11 at r2 (raw file):

## Upgrading to version 9

### With no interest of integrating webpacker

Need a few sentences of pros and cons of either way.


docs/basics/upgrading-react-on-rails.md, line 16 at r2 (raw file):

  • In /config/initializers/react_on_rails.rb, rename:
    • config.npm_build_test_command ==> config.build_test_command
    • config.npm_build_production_command ==> config.build_production_command
  • Also create config.node_modules_location and set it to "client"
* In `/config/initializers/react_on_rails.rb`:
    *  Rename `config.npm_build_test_command` ==> `config.build_test_command`
    *  Rename `config.npm_build_production_command` ==> `config.build_production_command`
    *  Add `config.node_modules_location = "client"`

docs/basics/upgrading-react-on-rails.md, line 23 at r2 (raw file):

Reason for doing this: This enables your webpack bundles to bypass the Rails asset pipeline and it's extra minification, enabling you to use source-maps in production, while still maintaining total control over everything in the client directory

#### From version 8

Most people are upgrading from 7 or earlier to 9 or later, so this is a bit confusing.


Comments from Reviewable

@justin808
Copy link
Member

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


Comments from Reviewable

@justin808
Copy link
Member

suggestions provided


Review status: all files reviewed at latest revision, 6 unresolved discussions.


docs/basics/upgrading-react-on-rails.md, line 32 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Well, if we're going to tell people to revert it, then we'll have to identify exactly what changes RoR made to it. Shall we assume the client installed RoR v7?

No idea on this one.


Comments from Reviewable

@Judahmeek
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 6 unresolved discussions.


docs/basics/upgrading-react-on-rails.md, line 20 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

This makes no sense at all to me.

Webpacker provides 2 things:

  1. View helpers that support bypassing the asset pipeline to enable things like avoiding double minification and proper source maps. This is 100% a best practice.
  2. A default Webpack config so that you only need to do minimal modifications and customizations. However, if you're doing server rendering, you may not want to give up control. Since Webpacker's default webpack config is changing often, we can give you definitive advice on this one. In general, if you're happy with doing your own Webpack configuration, then stick to that. Most bigger projects will prefer having more control.

Stolen


docs/basics/upgrading-react-on-rails.md, line 11 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Need a few sentences of pros and cons of either way.

Used your own comments with minor modifications


docs/basics/upgrading-react-on-rails.md, line 16 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…
  • In /config/initializers/react_on_rails.rb, rename:
    • config.npm_build_test_command ==> config.build_test_command
    • config.npm_build_production_command ==> config.build_production_command
  • Also create config.node_modules_location and set it to "client"
* In `/config/initializers/react_on_rails.rb`:
    *  Rename `config.npm_build_test_command` ==> `config.build_test_command`
    *  Rename `config.npm_build_production_command` ==> `config.build_production_command`
    *  Add `config.node_modules_location = "client"`

Done


docs/basics/upgrading-react-on-rails.md, line 23 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Most people are upgrading from 7 or earlier to 9 or later, so this is a bit confusing.

I moved this section to the end


Comments from Reviewable

@justin808
Copy link
Member

@Judahmeek Any final questions for me? Ready to merge?


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


Comments from Reviewable

@Judahmeek
Copy link
Contributor Author

@justin808 Yes, ready to merge.

@justin808 justin808 merged commit cc9db6d into master Oct 26, 2017
@justin808 justin808 deleted the jmeek/v9-migration branch November 22, 2017 05:49
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.

2 participants