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

Justin808/webpacker integration #822

Merged
merged 24 commits into from
May 6, 2017
Merged

Conversation

justin808
Copy link
Member

@justin808 justin808 commented Apr 26, 2017

This builds on #811.

This change is Reviewable

@justin808 justin808 force-pushed the justin808/webpacker_integration branch 2 times, most recently from 3cea6dc to 791a68b Compare April 26, 2017 10:16
const { env, paths, publicPath } = webpackConfigLoader(resolve('..', 'config', 'webpack'));
const webpackConfigLoader = require('react-on-rails/node_package/lib/webpackConfigLoader').default;
const configPath = resolve('..', 'config', 'webpack');
const { env, paths, publicPath } = webpackConfigLoader(configPath);
Copy link
Member Author

Choose a reason for hiding this comment

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

@alexfedoseev @robwise Does this look OK?

Copy link
Member

Choose a reason for hiding this comment

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

@justin808 What are your concerns here? JS looks ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexfedoseev are you okay with making everyone modify their webpack configs to import env, paths, and publicPath from this react-on-rails lib for webpackConfigLoader instead of setting themselves manually as normal?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to change this around a bit as there seems to be no reason to have the webpack config helper go through babel. Thus, I can move the file so that the

require('react-on-rails/node_package/lib/webpackConfigLoader')

is simpler, like this:

require('react-on-rails/webpackConfigLoader')

Copy link
Member

Choose a reason for hiding this comment

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

@alexfedoseev are you okay with making everyone modify their webpack configs to import env, paths, and publicPath from this react-on-rails lib for webpackConfigLoader instead of setting themselves manually as normal?

Sounds like unnecessary and leaky layer.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alexfedoseev and @robwise I have no idea on what you're referring to. The only reason to modify the webpack configs is so that the webpacker_lite view helper will be able to go the public output directory and thus bypass the asset pipeline.

@justin808
Copy link
Member Author

CC: @kaizencodes be sure to check out any commits here.

@justin808
Copy link
Member Author

@kaizencodes

image

package.json
image

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.943% when pulling 33335aa on justin808/webpacker_integration into 5e4f5c5 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.943% when pulling 33335aa on justin808/webpacker_integration into 5e4f5c5 on master.

@justin808 justin808 force-pushed the justin808/webpacker_integration branch from aa621eb to e4810b6 Compare April 27, 2017 10:08
@justin808
Copy link
Member Author

justin808 commented Apr 27, 2017

@kaizencodes I've released the beta:

Gem: 7.1.0.beta.2
Npm: 7.1.0-beta.2

CC: @dzirtusss @udovenko @Judahmeek @robwise

You shouldn’t need to change anything at all besides the versions. I want to know if this switch to webpacker for the generators breaks existing apps first.

Next step will be a README for the migration to user webpacker_lite and to avoid the asset pipeline.

kaizencodes and others added 18 commits April 30, 2017 10:46
- 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
@justin808 justin808 force-pushed the justin808/webpacker_integration branch from eb84ec5 to 95765e3 Compare April 30, 2017 20:53
@justin808
Copy link
Member Author

I grabbed @kaizencodes latest commit on #811 and rebased my branch on master.

PENDING:

This should result in no pending changes after tests run locally.
@coveralls
Copy link

coveralls commented Apr 30, 2017

Coverage Status

Coverage decreased (-0.08%) to 97.943% when pulling 42c9ff7 on justin808/webpacker_integration into 4c4a27f on master.

@coveralls
Copy link

coveralls commented Apr 30, 2017

Coverage Status

Coverage decreased (-0.08%) to 97.943% when pulling 4316e22 on justin808/webpacker_integration into 4c4a27f on master.

@coveralls
Copy link

coveralls commented Apr 30, 2017

Coverage Status

Coverage decreased (-0.08%) to 97.943% when pulling 4316e22 on justin808/webpacker_integration into 4c4a27f on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.943% when pulling 84811ff on justin808/webpacker_integration into 4c4a27f on master.

* 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
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 97.967% when pulling bc257a7 on justin808/webpacker_integration into 4c4a27f on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 97.967% when pulling bc257a7 on justin808/webpacker_integration into 4c4a27f on master.

* 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)/,
@coveralls
Copy link

coveralls commented May 3, 2017

Coverage Status

Coverage decreased (-0.06%) to 97.967% when pulling bac53b1 on justin808/webpacker_integration into 4c4a27f on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 97.967% when pulling bac53b1 on justin808/webpacker_integration into 4c4a27f on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 97.967% when pulling bac53b1 on justin808/webpacker_integration into 4c4a27f on master.

@justin808 justin808 merged commit fcd6e28 into master May 6, 2017
@justin808 justin808 deleted the justin808/webpacker_integration branch May 6, 2017 09:07
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.

5 participants