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

Upgrade frontend build system to mix #597

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
2 participants
@mauricew
Contributor

mauricew commented Sep 27, 2017

Overview

Laravel now uses a slimmer build system that makes the process smaller and simpler. Gulp and bower are both gone, and npm handles all dependencies while webpack handles the build process. Absolutely no actual code should change or upgrades to dependencies made. Some unused files were also removed.

If there are any additional changes that should be made for technical reasons, let me know.

Checklist

  • Read the CONTRIBUTING document before submitting your PR.
  • If the PR is related to an issue or fix one, don't forget to indicate it.
  • Make sure that the change you propose is the smallest possible. (It isn't.)
  • Screenshots are included if the PR changes the UI. (It should not)
  • Tests added for this feature/bug. (no actual site changes)
  • CHANGELOG entry added, if necessary, under UNRELEASED.
  • CONTRIBUTORS entry added, if necessary.
  • Indicate [wip] in the title of the PR it is is not final yet. Remove [wip] when ready. Otherwise the PR will be considered complete and rejected if it's not working.
@djaiss

This comment has been minimized.

Show comment
Hide comment
@djaiss

djaiss Oct 28, 2017

Member

@mauricew I totally disregarded this PR. So sorry about that.

Let me read more about mix to see how it will affect the codebase.

Member

djaiss commented Oct 28, 2017

@mauricew I totally disregarded this PR. So sorry about that.

Let me read more about mix to see how it will affect the codebase.

@mauricew

This comment has been minimized.

Show comment
Hide comment
@mauricew

mauricew Oct 28, 2017

Contributor

Two quickest things I can say about it are

  • Use of webpack for all of the css & js - You can get rid of gulp if you want, but more importantly bower being removed is a good thing.
  • When the js files are regenerated, the file name doesn't change - it gets appended to the rev-manifest.json file. This is pretty good for source control.
Contributor

mauricew commented Oct 28, 2017

Two quickest things I can say about it are

  • Use of webpack for all of the css & js - You can get rid of gulp if you want, but more importantly bower being removed is a good thing.
  • When the js files are regenerated, the file name doesn't change - it gets appended to the rev-manifest.json file. This is pretty good for source control.

@mauricew mauricew closed this Oct 28, 2017

@mauricew

This comment has been minimized.

Show comment
Hide comment
@mauricew

mauricew Oct 28, 2017

Contributor

Oops I accidentally closed it...

I'll make a new commit with the latest from master

Contributor

mauricew commented Oct 28, 2017

Oops I accidentally closed it...

I'll make a new commit with the latest from master

@mauricew mauricew reopened this Oct 28, 2017

@djaiss

This comment has been minimized.

Show comment
Hide comment
@djaiss

djaiss Oct 28, 2017

Member

@mauricew

When the js files are regenerated, the file name doesn't change - it gets appended to the rev-manifest.json file. This is pretty good for source control.

Hum. Name won't change? That might be a problem for cache invalidation.

Member

djaiss commented Oct 28, 2017

@mauricew

When the js files are regenerated, the file name doesn't change - it gets appended to the rev-manifest.json file. This is pretty good for source control.

Hum. Name won't change? That might be a problem for cache invalidation.

@mauricew

This comment has been minimized.

Show comment
Hide comment
@mauricew

mauricew Oct 28, 2017

Contributor

It gets appended when being served through http, just not directly in the filesystem.

Before: /build/css/app-1234567.css
After: /css/app.css?id=1234567 (autogenerated whenever it changes)

Contributor

mauricew commented Oct 28, 2017

It gets appended when being served through http, just not directly in the filesystem.

Before: /build/css/app-1234567.css
After: /css/app.css?id=1234567 (autogenerated whenever it changes)

@djaiss

3 comments so far. Excellent job, I believe this will really help us moving forward with the project.

As a side note, I had to do this to make it work:

rm -rf node_modules
rm package-lock.json yarn.lock
npm cache clear --force
npm install
Show outdated Hide outdated package.json Outdated
Show outdated Hide outdated resources/assets/js/app.js Outdated
Show outdated Hide outdated package.json Outdated
@mauricew

This comment has been minimized.

Show comment
Hide comment
@mauricew

mauricew Nov 2, 2017

Contributor

A few things:

  • Documentation has a whole section on bower so I don't know how it should be changed
  • Dockerfile has been changed
  • It seems to be suggested that package/yarn.lock files are checked into the repo, I wasn't sure before if you were using npm@5
Contributor

mauricew commented Nov 2, 2017

A few things:

  • Documentation has a whole section on bower so I don't know how it should be changed
  • Dockerfile has been changed
  • It seems to be suggested that package/yarn.lock files are checked into the repo, I wasn't sure before if you were using npm@5
@djaiss

This comment has been minimized.

Show comment
Hide comment
@djaiss

djaiss Nov 2, 2017

Member

@mauricew

  • You can omit the documentation if you want - or if you know what we should remove about Bower, do it. I'll update it in case you don't.
  • Dockerfile will have to be changed indeed. If you know how, you can do it, otherwise I'll take care of it also.
  • I actually don't know what to do with Yarn. Are you still using it in this PR? I don't think you do. Can we safely remove it? I really am not familiar with this whole JS universe, unfortunately, but I'm eager to learn.
Member

djaiss commented Nov 2, 2017

@mauricew

  • You can omit the documentation if you want - or if you know what we should remove about Bower, do it. I'll update it in case you don't.
  • Dockerfile will have to be changed indeed. If you know how, you can do it, otherwise I'll take care of it also.
  • I actually don't know what to do with Yarn. Are you still using it in this PR? I don't think you do. Can we safely remove it? I really am not familiar with this whole JS universe, unfortunately, but I'm eager to learn.

@mauricew mauricew changed the title from [wip] Upgrade frontend build system to mix to Upgrade frontend build system to mix Nov 2, 2017

@mauricew

This comment has been minimized.

Show comment
Hide comment
@mauricew

mauricew Nov 2, 2017

Contributor

I'm using npm 5 but I'm not gonna commit a lockfile. Also I changed the Dockerfile (just removed the two lines having to do with bower)

Contributor

mauricew commented Nov 2, 2017

I'm using npm 5 but I'm not gonna commit a lockfile. Also I changed the Dockerfile (just removed the two lines having to do with bower)

@djaiss

djaiss approved these changes Nov 2, 2017

@djaiss djaiss merged commit 59d3531 into monicahq:master Nov 2, 2017

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@djaiss

This comment has been minimized.

Show comment
Hide comment
@djaiss

djaiss Nov 2, 2017

Member

Thanks so much @mauricew for your help!

Member

djaiss commented Nov 2, 2017

Thanks so much @mauricew for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment