-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Daniel/webpacker integration #811
Daniel/webpacker integration #811
Conversation
Great start!!!! Reviewed 26 of 26 files at r1. lib/generators/react_on_rails/templates/base/base/client/webpackConfigLoader.js, line 15 at r1 (raw file):
@kaizencodes Should we put this in the node library? so we don't have to include this with the generator? I think so. If this is in the library, we can have the exports come from a function call, so that if the configuration place is different, it can be set, or if the webpack config is in a different place. Essentially, have this be the default config path, but allow overriding it. lib/generators/react_on_rails/templates/base/base/config/webpack/paths.yml, line 19 at r1 (raw file):
this is AWESOME cc: @robwise, @alexfedoseev, @samnang spec/dummy/client/webpack.client.base.config.js, line 20 at r1 (raw file):
this line is maybe too wide, definitely we want to try to fi lines under 100 chars wide spec/dummy/client/webpack.client.base.config.js, line 39 at r1 (raw file):
remove comment? spec/dummy/client/webpackConfigLoader.js, line 22 at r1 (raw file):
this could come from the node file, just like the generator file so that would DRY up things spec/dummy/config/initializers/react_on_rails.rb, line 100 at r1 (raw file):
that needs to be the default for new projects --> did that change for the generator? In fact, we should consider removing this from generated file, and the default is nil. I think for here, it's OK to specify it. However, that will not confirm that the default is nil, possibly. Comments from Reviewable |
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke. lib/generators/react_on_rails/templates/base/base/config/webpack/paths.yml, line 2 at r1 (raw file):
I still don't get how I set it to build two configs (server-rendering and client). I've said this over and over and no one ever responds. Why does webpacker even need to know about my webpack config? Comments from Reviewable |
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke. lib/generators/react_on_rails/templates/base/base/config/webpack/paths.yml, line 2 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
@robwise It needs to know where the manifest file is, to include the assets. Why wouldn't it work for 2 configs? Does the output differ? Comments from Reviewable |
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke. lib/generators/react_on_rails/templates/base/base/config/webpack/paths.yml, line 2 at r1 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
@kaizencodes It wouldn't work for two configs because it only takes a single string as an argument and not an array, so how do I put two configs there? We have a server bundle config and a client bundles config for server rendering. We also have a DLL config for development. Which one of the three do I put here? What happens to the other two? Comments from Reviewable |
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke. lib/generators/react_on_rails/templates/base/base/config/webpack/paths.yml, line 2 at r1 (raw file): Previously, robwise (Rob Wise) wrote…
@robwise Okay, now I get where you are getting at. First of this was meant to show a dir path where all the configs are located. But we don't use this 'config' key anymore (Should have deleted it already). The procfiles load the appropriate configs, each loads this shared file. Comments from Reviewable |
@kaizencodes Let's delete any unnecessary configs, and also make it clear in the comments of the config files that webpacker_lite differs from webpacker in that you have to provide your own config. |
Shouldn't it be better to place this info in the webpacker_lite readme? Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke. lib/generators/react_on_rails/templates/base/base/client/webpackConfigLoader.js, line 15 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
I'm not sure how this would fit in the node package. spec/dummy/config/initializers/react_on_rails.rb, line 100 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
nil is the default. However, This is from the template:
Comments from Reviewable |
Couple questions Reviewed 3 of 3 files at r2. spec/dummy/client/webpack.client.base.config.js, line 19 at r2 (raw file):
I'm not clear on this code or the error message. Please explain. What is the sharedManifest variable for? Seems to be some sort of caching thing. spec/dummy/client/webpack.server.rails.build.config.js, line 22 at r2 (raw file):
is this error ever expected, as in the first time this is run? Comments from Reviewable |
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks broke. spec/dummy/client/webpack.client.base.config.js, line 19 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
Yes, it is caching. When we use 2 configs like client and server, each will write to the manifest. However, if we do not include the hash created by the first, the second will overwrite it. spec/dummy/client/webpack.server.rails.build.config.js, line 22 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
yes, it occurs with the first config always. Comments from Reviewable |
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke. lib/generators/react_on_rails/templates/base/base/client/webpackConfigLoader.js, line 15 at r1 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
This could be a non-default import in the node package. import webpackConfigLoader from 'react-on-rails/webpackConfigLoader' spec/dummy/client/webpack.client.base.config.js, line 19 at r2 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
Maybe the message should only be printed if in a debugging mode? Maybe the message should be clear that this prints the first time the config is run? Comments from Reviewable |
some comments Reviewed 12 of 12 files at r3. lib/generators/react_on_rails/templates/base/base/client/webpack.config.js, line 7 at r3 (raw file):
This seems odd to need the eslint disable here. Any idea on why? lib/generators/react_on_rails/templates/base/base/client/webpackConfigLoader.js, line 15 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
@robwise what do you think about putting this in the react-on-rails package? The one issue is that we'd have to pass in a param of the location of the config/webpack/paths.yml file. spec/dummy/app/views/layouts/application.html.erb, line 24 at r1 (raw file):
why are we removing the turbo tag? spec/dummy/app/views/layouts/application.html.erb, line 20 at r3 (raw file):
should this comment be here spec/dummy/client/webpack.client.base.config.js, line 37 at r3 (raw file):
this seems odd b/c we also have react-on-rails listed in the package.json spec/dummy/client/webpack.server.rails.build.config.js, line 39 at r3 (raw file):
this seems odd b/c we also have react-on-rails listed in the package.json spec/dummy/config/initializers/assets.rb, line 14 at r3 (raw file):
very nice! this needs to be well documented! is this in the generator as well? it should be! spec/dummy/config/webpack/development.server.yml, line 8 at r3 (raw file):
so you're turning off hot reloading? Comments from Reviewable |
Review status: 24 of 29 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed. lib/generators/react_on_rails/templates/base/base/client/webpack.config.js, line 7 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
eslint couldn't resolve it. Which is understandable given we don't have the package in the generator. spec/dummy/client/webpack.client.base.config.js, line 37 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
yes, it is needed when we require it locally. spec/dummy/client/webpack.server.rails.build.config.js, line 39 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
yes, it is needed when we require it locally. spec/dummy/config/webpack/development.server.yml, line 8 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
worked with static last time, hot loading should be the default? Comments from Reviewable |
Reviewed 6 of 6 files at r4. lib/generators/react_on_rails/base_generator.rb, line 62 at r4 (raw file):
I think this is going to blow away any existing application.html.erb. The generator will be used on existing projects. How about we make a new layout and set that in the controller for hello_world? lib/generators/react_on_rails/dev_tests_generator.rb, line 56 at r4 (raw file):
I don't get why we need an alias for this. The generators will generate code that depend on the release of react-on-rails corresponding to the gem version. lib/generators/react_on_rails/templates/base/base/app/views/layouts/application.html.erb.tt, line 10 at r4 (raw file):
This file can be renamed so as not to conflict with any existing application.html.erb. lib/generators/react_on_rails/templates/base/base/client/webpack.config.js, line 7 at r3 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
hmmm....we don't want the generated code to have that unnecessary disable. I think we can add the package globally /package.json spec/dummy/client/webpack.client.base.config.js, line 37 at r3 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
@kaizencodes But we didn't change this for 7.0, so we should not be adding this. spec/dummy/config/webpack/development.server.yml, line 8 at r3 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
I'm really not sure on this one. Can this setting be overriden with an env value? Comments from Reviewable |
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed. lib/generators/react_on_rails/dev_tests_generator.rb, line 56 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
I don't get it either, yet without it react-on-rails fails to resolve. spec/dummy/config/webpack/development.server.yml, line 8 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
We could. Should we? Comments from Reviewable |
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed. lib/generators/react_on_rails/dev_tests_generator.rb, line 56 at r4 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
this is what I get on master:
Comments from Reviewable |
1abd20a
to
9c3926f
Compare
Review status: 27 of 32 files reviewed at latest revision, 17 unresolved discussions. spec/dummy/config/webpack/development.server.yml, line 8 at r3 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
We should, this caused the travis error. I've set up the config loader to overwrite this value when REACT_ON_RAILS_ENV is HOT. Now the question comes: Do we need this? Is there a scenario where we need the dev server but not hot reload? Comments from Reviewable |
rebased onto master Review status: 27 of 32 files reviewed at latest revision, 17 unresolved discussions. Comments from Reviewable |
more comments Reviewed 6 of 6 files at r5. .eslintignore, line 15 at r5 (raw file):
I don't think we should do this. It's good to lint these. Did you try installing at the top level, the missing dependency? lib/generators/react_on_rails/dev_tests_generator.rb, line 56 at r4 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
this is odd We definitely can't put this in. For one, the generator won't work outside of within the REact on Rails source! lib/generators/react_on_rails/templates/base/base/app/controllers/hello_world_controller.rb, line 2 at r5 (raw file):
I think we should rename all the "hello_world" to "hello_react_on_rails" lib/generators/react_on_rails/templates/base/base/client/webpack.config.js, line 7 at r3 (raw file): Previously, justin808 (Justin Gordon) wrote…
hey, package should be in the generated package.json. Maybe that's the issue. And not a dev, but a regular dependency. spec/dummy/client/webpackConfigLoader.js, line 10 at r5 (raw file):
let's put a comment in the yml file about this env value and I still want this whole thing in the node package spec/dummy/client/webpackConfigLoader.js, line 14 at r5 (raw file):
excellent! spec/dummy/config/webpack/development.server.yml, line 8 at r3 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
hot == dev server Comments from Reviewable |
Review status: all files reviewed at latest revision, 20 unresolved discussions, some commit checks failed. lib/generators/react_on_rails/templates/base/base/app/controllers/hello_world_controller.rb, line 2 at r5 (raw file): Previously, justin808 (Justin Gordon) wrote…
Should we? it's all over the place Comments from Reviewable |
getting there! see comments Reviewed 19 of 19 files at r6. package.json, line 44 at r6 (raw file):
what is this part for? lib/generators/react_on_rails/dev_tests_generator.rb, line 56 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
did you figure this out? maybe I can help today. lib/generators/react_on_rails/dev_tests_generator.rb, line 55 at r6 (raw file):
remove comments? lib/generators/react_on_rails/templates/base/base/app/controllers/hello_world_controller.rb, line 2 at r5 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
actually, this could be a separate PR -- keep this the same for now lib/generators/react_on_rails/templates/base/base/client/webpack.config.js, line 8 at r6 (raw file):
any way we can get this to pass without the eslint issue? I think maybe we can list react-on-rails in the top level package.json, and use a relative file ref and we need to change this instead the require needs to be like
rather than the default lib/generators/react_on_rails/templates/base/base/config/webpack/development.server.yml, line 8 at r6 (raw file):
we should put a comment here to make hot reloading the default, you can change this line to true something like that, that is clear and short node_package/src/ReactOnRails.js, line 11 at r6 (raw file):
we need to remove this instead the require needs to be like
rather than the default node_package/src/ReactOnRails.js, line 227 at r6 (raw file):
this should not go in the default react on rails package node_package/src/webpackConfigLoader.js, line 8 at r6 (raw file):
remove comments const configLoader = (configPath) => {
return something;
}
export default configLoader; spec/dummy/client/webpack.client.base.config.js, line 40 at r6 (raw file):
remove comments spec/dummy/client/webpack.client.rails.hot.config.js, line 11 at r6 (raw file):
see comments above spec/dummy/client/webpack.server.rails.build.config.js, line 40 at r6 (raw file):
remove comments spec/react_on_rails/generators/dev_tests_generator_spec.rb, line 47 at r6 (raw file):
remove? Comments from Reviewable |
Review status: 27 of 38 files reviewed at latest revision, 31 unresolved discussions, some commit checks failed. package.json, line 44 at r6 (raw file): Previously, justin808 (Justin Gordon) wrote…
couldn't resolve fs that is in the configLoader lib/generators/react_on_rails/dev_tests_generator.rb, line 56 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
yes I did, needed to run npm i on the react-on-rails package. which is odd as it seems the rake tasks should do that. spec/react_on_rails/generators/dev_tests_generator_spec.rb, line 47 at r6 (raw file): Previously, justin808 (Justin Gordon) wrote…
I wasn't sure if we need this somewhere or not Comments from Reviewable |
Good progress. Definitely some more to do! Reviewed 1 of 19 files at r6, 11 of 11 files at r7. package.json, line 44 at r6 (raw file): Previously, kaizencodes (Daniel Szatmari) wrote…
yes -- this should be removed, so that we're not loading the config file in the browser. rakelib/run_rspec.rake, line 125 at r7 (raw file):
this might be OK for a bit we need to investigate if the test helper can do this automatically spec/dummy/client/server-rails-hot.js, line 22 at r7 (raw file):
this should be a relative require spec/dummy/client/webpack.server.rails.build.config.js, line 11 at r7 (raw file):
const configPath = resolve('..', 'config', 'webpack');
const { env, paths, publicPath } = webpackConfigLoader(configPath); spec/dummy/config/initializers/react_on_rails.rb, line 21 at r7 (raw file):
we should not be removing the css files! those should be generated Comments from Reviewable |
Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed. spec/dummy/config/initializers/react_on_rails.rb, line 21 at r7 (raw file): Previously, justin808 (Justin Gordon) wrote…
Actually, we didn't have them there before. Comments from Reviewable |
0b18d2b
to
e77d098
Compare
- add alias for gen examples
* Any version mismatch of gem and node package throws an error. * 6.9 introduced code that would parse the props into a Hash unnecessarily. Any customers with significant String props took a performance hit from this on page rendering. * Fix is to only put props inside of the script tag, and not any meta data, like the dom id and name of the component. This avoids unnecessary parsing.
* No need to use babel on this file * Add another .eslintrc to avoid error on missing require * Configured package.json to export the file webpackConfigLoader.js
* New default dir reflects generators
I grabbed your latest commit and rebased my branch on master. PENDING:
|
This should result in no pending changes after tests run locally.
* Removed unnecessary rake task for running specs * Fixed inconsistency with using "-bundle" only some of the time. * Removed unused node_modules value in paths.yml * Upgraded to webpacker_lite 1.0.0
* Added CHANGELOG.md entry * New default for symlink_non_digested_assets_regex is nil for webpacker_lite default. Previously: symlink_non_digested_assets_regex: /\.(png|jpg|jpeg|gif|tiff|woff|ttf|eot|svg|map)/,
e77d098
to
ceaeb7e
Compare
1 similar comment
Review status: 21 of 48 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed. docs/basics/migrating-to-webpacker.md, line 38 at r12 (raw file):
you never closed this if you looked at your file as rendered in markdown, this is obvious. docs/basics/migrating-to-webpacker.md, line 44 at r12 (raw file):
const webpackConfigLoader = require('react-on-rails/webpackConfigLoader');
const configPath = resolve('..', 'config', 'webpack');
const { paths, publicPath } = webpackConfigLoader(configPath); Comments from Reviewable |
This PR is still open b/c of the doc commit. @kaizencodes Please move that to a new PR. |
This change is