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

Removes Babel duplication in Webpack configs #435

Merged
merged 3 commits into from
Jan 9, 2019

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas commented Dec 17, 2018

What this PR does / why we need it:

Webpacker manages webpack itself, creating config files at compilation time. However this generated configs are being overridden in porta in a hardcoded way that leads to errors and uncontrolled results.

This PR gives this control back to Webpacker and Rails and apply all custom configs appropriately. This way config files look a bit more convoluted but also free us from managing webpack.js files.

Read more about how to manage webpack config with webpacker here.

Thanks to this change we are able to better handle specific setups (dev, test, prod..) as well as reduce duplicated code in a safer and more maintainable way by means of JS.

@josemigallas josemigallas self-assigned this Dec 17, 2018
@josemigallas josemigallas force-pushed the feature/unify_babel_webpack_config branch from 2254e8c to ccb15d7 Compare December 17, 2018 14:24
@josemigallas josemigallas force-pushed the feature/unify_babel_webpack_config branch from ccb15d7 to 42c9176 Compare December 17, 2018 15:02
@codecov
Copy link

codecov bot commented Dec 17, 2018

Codecov Report

Merging #435 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #435      +/-   ##
=========================================
+ Coverage   92.74%   92.8%   +0.05%     
=========================================
  Files        2341    2345       +4     
  Lines       75708   75919     +211     
=========================================
+ Hits        70212   70453     +241     
+ Misses       5496    5466      -30
Impacted Files Coverage Δ
lib/arel/visitors/oracle12.rb 72.72% <0%> (ø)
spec/acceptance/api/account_plan_spec.rb 100% <0%> (ø)
spec/acceptance/api/account_user_spec.rb 100% <0%> (ø)
spec/acceptance/api/account_spec.rb 100% <0%> (ø)
app/models/account/buyer_methods.rb 93.65% <0%> (+1.58%) ⬆️
lib/api_authentication/by_access_token.rb 95.76% <0%> (+4.23%) ⬆️
config/initializers/access_token_authentication.rb 100% <0%> (+33.33%) ⬆️
config/initializers/oracle.rb 50% <0%> (+48.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c145979...c73acfb. Read the comment docs.

Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

I'd say this is the moment to start using https://babeljs.io/docs/en/babel-preset-env, and get rid of es2015, stage-0, etc.

@josemigallas
Copy link
Contributor Author

@didierofrivia That's the purpose of #394 .. unfortunately it's not working at all, so I thought it would be better to make smaller changes each team, starting with this one.

@josemigallas josemigallas force-pushed the feature/unify_babel_webpack_config branch 2 times, most recently from cc937a8 to 254736b Compare December 18, 2018 14:45
Copy link
Contributor

@damianpm damianpm left a comment

Choose a reason for hiding this comment

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

It LGTM, what do ou think @didierofrivia ?

@josemigallas josemigallas force-pushed the feature/unify_babel_webpack_config branch 2 times, most recently from f2ac8c5 to c1247c3 Compare December 20, 2018 07:58
@didierofrivia
Copy link
Member

Looks good, still circle ci needs to bless this PR.

@josemigallas josemigallas force-pushed the feature/unify_babel_webpack_config branch from c1247c3 to c73acfb Compare January 2, 2019 10:59
Copy link
Contributor

@hallelujah hallelujah left a comment

Choose a reason for hiding this comment

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

What I am not sure is if the bablerc will be exported in the image. If not we need tonverify that.

@josemigallas
Copy link
Contributor Author

@hallelujah Please feel free to approve if you think it is enough verification to have the docker-build job

@josemigallas josemigallas merged commit fe04900 into master Jan 9, 2019
@josemigallas josemigallas deleted the feature/unify_babel_webpack_config branch January 9, 2019 10:48
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