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

feature(webpack4) Migrate project to webpack4. Prepare for React 16 #1964

Merged
merged 7 commits into from
Apr 10, 2018

Conversation

EugeneHlushko
Copy link
Member

@EugeneHlushko EugeneHlushko commented Mar 27, 2018

  • Migrate to webpack 4
  • Deprecate redundant plugins/loaders
  • ready to migrate to react ^16..

Possibly fixes #1924
Unblocks #1967

"file-loader": "^0.11.2",
"fontgen-loader": "^0.2.1",
"file-loader": "^1.1.11",
"fontgen-loader": "git://github.com/EugeneHlushko/fontgen-loader.git#a26a73843900ca4b518853952b1fc3c816103512",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is valid until DragonsInn/fontgen-loader#42 is merged

package.json Outdated
"tap-min": "^1.2.1",
"tap-parser": "^6.0.1",
"through2": "^2.0.3",
"url-loader": "^0.5.9",
"webpack": "^3.10.0",
"webpack": "^4.2.0",
"webpack-dev-server": "^2.9.7",
"webpack-merge": "^4.1.0",
Copy link
Member Author

@EugeneHlushko EugeneHlushko Mar 27, 2018

Choose a reason for hiding this comment

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

i did not remove webpack-merge yet just in case, if no objections ill remove it after next update with target branch

antwar.config.js Outdated
'install-webpack': '/guides/installation',
'why-webpack': '/guides/why-webpack',
}
/*************************
Copy link
Member Author

Choose a reason for hiding this comment

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

@bebraw can you please review if redirects belong here? putting outside of parent breaks on getAllPages call

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on the version you are using. I dropped direct support for redirects recently. I use redirect-webpack-plugin instead so maybe you want to migrate to that. It's possible to do that even with an older version of Antwar although the new one is nicer (faster etc.).

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the newest version of Antwar runs on webpack 4.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats exactly what im doing here, moving to antwar 0.28.3 and to webpack 4, so i will use redirect-webpack-plugin instead, thank you!

new MiniCssExtractPlugin({
filename: '[chunkhash].css'
}),
new RedirectWebpackPlugin({
Copy link
Member Author

Choose a reason for hiding this comment

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

@bebraw thank you man, works like a charm

@EugeneHlushko
Copy link
Member Author

@jeremenichelli @montogeek guys can you review this one? LGTM so far

@jeremenichelli
Copy link
Member

This is great, now I don't know if we should merge it since @skipjack has been working on a infrastructure change in the rebuild branch.

@EugeneHlushko can you connect with him about this? I want to make sure that merging this doesn't affect the improvements Greg has done or if you can bring some of this changes to that branch.

@montogeek
Copy link
Member

I have been working on rebuild branch, these PR changes won't affect it, We still need to upgrade to webpack 4, specially webpack website :D. We could merge later or even better make these upgrade against rebuild branch, It is possible you will find some blockers with 2 plugins that are new and may not been migrated to webpack 4 .

@skipjack
Copy link
Collaborator

skipjack commented Mar 28, 2018

So I don't think any of these changes will be a huge problem for the rebuild branch but they will create some conflicts to resolve. Here's some notes on the rebuild branch:

  • It's already on React/ReactDOM 16
  • We haven't done any preact / inferno replacement as the bundles are already <=75kb gzipped (and there's likely still room for other optimizations)
  • Antwar and it's utilities are no longer used on rebuild, it's pure webpack
  • We may be able to leverage the RedirectWebpackPlugin
  • For a full list of notes and TODOs, see the DISCUSSION.md file on that branch
  • We discussed waiting to port rebuild to webpack v4 until after the remaining TODOs are finished. Few of the remaining items relate to webpack and the build process and trying to do the port now would just add more complexity. With more control over configs and the site in general, it'll be much easier for us to do targeted updates of dependencies faster and in isolation.

I would say if you all see improvements here and get the build to pass, then feel free to merge. After this though, if the plan is to finish and merge rebuild, let's not make any more conflicting changes and instead just focus on finishing rebuild. If you see something in the DISCUSSION.md file you'd like to tackle, please ping me to discuss so we can agree on plan and don't overlap work. The ones @montogeek and I are working on are already marked at the bottom of the list.

@EugeneHlushko
Copy link
Member Author

This is a shame that there was a branch with changes in this direction, i did look the issues section before starting the work :( Would be a shame to drop it for me:

  • as its ready to go
  • removes preact
  • seems to not have too much conflict on rebuild (wish i knew it existed)

But yes please decide @montogeek @skipjack

@EugeneHlushko
Copy link
Member Author

As a side note to think about, i really would like to see more incremental changes vs big branches. Especially considering such good CI and CD integrations, it would make it easier and more transparent not only for team members but also for the community

@jeremenichelli
Copy link
Member

jeremenichelli commented Mar 29, 2018

I agree with you on your last point @EugeneHlushko, but to be fair the site had no maintainers at all when Greg started its work on rebuild branch and I'm not expecting to have a refactor every year. So yeah, incremental work for the win.

If Greg agrees there are not major issues then we should make sure the site works entirely with these changes, fix the integration issues and merge 🥇

@EugeneHlushko
Copy link
Member Author

@jeremenichelli If you mean continuous-integration/travis-ci/pr it unfortunately fails due to api limit exceeded error :(

[Error: {"message":"API rate limit exceeded for 35.188.1.99. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://developer.github.com/v3/#rate-limiting"}]

@montogeek
Copy link
Member

montogeek commented Mar 29, 2018

@EugeneHlushko This branch have latest master changes?
Edit: yes

@montogeek
Copy link
Member

We are hitting this Travis Security measure: https://docs.travis-ci.com/user/pull-requests#Pull-Requests-and-Security-Restrictions.
Maybe we could use cache artifacts, need investigation

@montogeek
Copy link
Member

@EugeneHlushko You are a member, Move your branch to this repo :)

@skipjack
Copy link
Collaborator

This is a shame that there was a branch with changes in this direction, i did look the issues section before starting the work

I thought there was a mention of it in #1525, I'll double check and add one now if there's not. Sorry about that.

Would be a shame to drop it for me...

I understand, I wasn't saying it should be dropped.

As a side note to think about, i really would like to see more incremental changes vs big branches. Especially considering such good CI and CD integrations, it would make it easier and more transparent not only for team members but also for the community

Totally agree. I started the rebuild branch as more of an experiment but I (mistakenly) let it get bigger than it should have. It does build the site extremely quick however and makes tackling a lot of our dev-related issues much easier since it's pure webpack (giving us more direct control). Recently, I made a short list of the absolute minimum we need to do to complete it and, with other folks like @montogeek helping, I think we'll be able to get it done soon. Finishing #1582 would also be good so we can better test the site for link and missing page issues.

I discussed this with @montogeek and @jeremenichelli and it seems like they are interested in pushing it forward since it's fairly close and offers a lot of what we want. We may still port to gatsby or another PWA/SS generator that requires less local config down the road but that port should be much easier after this one. Certain aspects of Antwar's API, e.g. the Interactive component (as well as our use of preact, led to non-standard React code that we can move away from on the rebuild branch (and hopefully stay away from in any other generator we choose).

On a side note, @EugeneHlushko seeing as you've submitted a lot in the last few months, I'd be happy to add you as a contributor to the project. We can also ping @TheLarkInn to add you to the slack channel.

it unfortunately fails due to api limit exceeded error

Yeah I thought @montogeek had fixed that in #1963 but it sounds like it may be another issue. I think faster builds will also help us run up avoid hitting Travis limits.

@EugeneHlushko
Copy link
Member Author

Nice! no more forks here for me in that case @montogeek 👍

Thank you @skipjack appreciate everything said above! Will join slack when Sean invites

@EugeneHlushko
Copy link
Member Author

CC @montogeek @jeremenichelli i've just rebased it and resolved conflicts

@EugeneHlushko
Copy link
Member Author

@montogeek there is another conflict now, does it worth resolving?

@montogeek
Copy link
Member

montogeek commented Apr 10, 2018

@EugeneHlushko Yes, I can do it

@EugeneHlushko
Copy link
Member Author

@montogeek i can resolve too np 👍 just wanted to check if we are merging this soon because last one was resolved yesterday :)

@EugeneHlushko
Copy link
Member Author

@montogeek its good again

@montogeek montogeek merged commit b45b73e into webpack:master Apr 10, 2018
@montogeek
Copy link
Member

Thank you @EugeneHlushko !

@EugeneHlushko EugeneHlushko deleted the feature/webpack4 branch April 11, 2018 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken image on website
6 participants