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

Switch to laravel-mix #2011

Closed
wants to merge 3 commits into from
Closed

Switch to laravel-mix #2011

wants to merge 3 commits into from

Conversation

QWp6t
Copy link
Sponsor Member

@QWp6t QWp6t commented Dec 24, 2017

Ideally I'd like to check these boxes before merging.

The following still needs to be fixed/added...

  • support for css injection (it injects, but then reloads anyway)
  • support for js injection + hmr (not super important, tbh)
  • vendor extractions for supported presets (bootstrap, foundation, etc)
  • restore support for user-defined cache-busting naming convention (e.g., [name]_[hash:8])

The following still needs to be tested (these should be simple to test, i just didn't do it yet)...

  • watch mode: adding background image (does image copy to dist?)
  • watch mode: removing background image (does image get removed from dist?)
  • copied images that aren't referenced in css/js should get cache-busted
  • source maps

@QWp6t
Copy link
Sponsor Member Author

QWp6t commented Dec 28, 2017

Just to keep everyone in the loop, we're hesitant to merge this now because preliminary benchmarks are showing that laravel-mix is significantly slower at compiling compared to our current setup. I'm not sure exactly what's different. I'm looking into it. But we're talking about differences of 10-20s, far too substantial to ignore.

@dmgawel
Copy link
Contributor

dmgawel commented Dec 31, 2017

@QWp6t CSS URL processing in Mix is known for its slow performance in some cases. Did you tried with this option disabled? laravel-mix/laravel-mix#49 (comment)

@retlehs
Copy link
Sponsor Member

retlehs commented Dec 31, 2017 via email

@myleshyson
Copy link

myleshyson commented Feb 13, 2018

This looks awesome so far! Just out of curiosity though, are the benchmarks based off just the initial install of sage or are you using a sample project? When I pulled in the laravel-mix branch I found that there isn't much difference in compiling time, but then again I'm just using the base sage theme.

It seemed like currently there were two seperate scripts running with the laravel-mix branch: npm lint -s and then the build script. When I removed that pre-build script and replaced it by adding eslint and styleint to the webpack configuration, it seemed to start compiling faster.

Here's the compiling times on my laptop.

Build Script Laravel Mix Sage Setup
build 6793ms 1623ms
build:production 18734ms 18354ms
start (initial) 4413ms 6578ms
start (small css change) 2749ms 3207ms

The only major difference for me was the build script which I don't use much anyway.

After this test I also removed the jquery imports from both customizer.js and main.js, since webpack autoloads that to every file anyway. That also seemed to speed up the compiling a little bit as well. I hope this helps!

@apintocr
Copy link

Actually this is amazing.
I'd say many of Sage 9 devs are also working with Laravel, this will put the workflows even closer together.

I've tested and it worked without any issues out of the box.

@wilburpowery
Copy link

Any ETA on this change being merged?

@retlehs
Copy link
Sponsor Member

retlehs commented Nov 16, 2018

webpack 4 support has landed in #2122. laravel mix might be waiting until webpack 5? this PR is stale and might be revisited in the future (or we might consider another package similar to mix)

@retlehs retlehs closed this Nov 16, 2018
@wilburpowery
Copy link

Yes, Jeffrey Way will wait until Webpack 5 before updating @retlehs.

@retlehs retlehs deleted the laravel-mix branch May 10, 2020 22:29
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.

None yet

6 participants