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

Fix hotloading #1209

Merged
merged 4 commits into from
Apr 18, 2020
Merged

Fix hotloading #1209

merged 4 commits into from
Apr 18, 2020

Conversation

Hyzual
Copy link
Contributor

@Hyzual Hyzual commented Apr 13, 2020

webpack's (major) changelog: https://github.com/webpack/webpack/releases/tag/v4.0.0
laravel-mix's (major) changelog: https://github.com/JeffreyWay/laravel-mix/releases/tag/v5.0.0

How to test:

How to use hot reload:

Note to reviewer:
EDIT: 8000 is now the port
- I chose 8088 as port number because that's what is expected by cypress tests. If another port is preferred, let me know.

@phanan
Copy link
Member

phanan commented Apr 13, 2020

Thanks, this looks great! Do we want to use the same port as CyPress though? I'd prefer another port for another env (dev instead of CyPress's test), for example, 8081.

@Hyzual
Copy link
Contributor Author

Hyzual commented Apr 13, 2020

Ok, let's use 8081. I'll amend the commit to change instructions.

webpack's (major) changelog: https://github.com/webpack/webpack/releases/tag/v4.0.0
laravel-mix's (major) changelog: https://github.com/JeffreyWay/laravel-mix/releases/tag/v5.0.0

How to test:
- yarn run production should succeed and build the assets

How to use hot reload:
- $ php artisan serve --port 8081
- $ yarn run hot
- Browse to http://localhost:8080
- Assets should be hot-reloaded. API calls should be proxied to your PHP
  server at http://localhost:8081
@phanan
Copy link
Member

phanan commented Apr 14, 2020

Hmm, now that I notice, why did Travis fail the build?

@phanan
Copy link
Member

phanan commented Apr 14, 2020

Thanks, this looks great! Do we want to use the same port as CyPress though? I'd prefer another port for another env (dev instead of CyPress's test), for example, 8081.

Sorry, I was an idiot. 8081 has been used for a (legacy) E2E test. Maybe we should go for a generic 8000. WDYT?

@Hyzual
Copy link
Contributor Author

Hyzual commented Apr 16, 2020

I've pushed another commit to change the port to 8000.
I'm used to amending commits but it looks like codecov didn't like that in koel/core so I'll stop force-pushing.

@phanan
Copy link
Member

phanan commented Apr 17, 2020

OK I finally have some time to dig a bit deeper into this. What do you think if we keep this change minimal? This should work:

--- app/Application.php
+++ app/Application.php
@@ -45,7 +45,7 @@ class Application extends IlluminateApplication
 
         if (isset($manifest[$file])) {
             return file_exists(public_path('public/hot'))
-                    ? "http://localhost:8080{$manifest[$file]}"
+                    ? "http://localhost:8080/public{$manifest[$file]}"
                     : $this->staticUrl("public{$manifest[$file]}");
         }
index 7755125..dc44964 100644
--- webpack.mix.js
+++ webpack.mix.js
@@ -9,6 +9,12 @@ mix.webpackConfig({
   output: {
     chunkFilename: mix.config.inProduction ? 'js/[name].[chunkhash].js' : 'js/[name].js',
     publicPath: '/public/'
+  },
+  devServer: {
+    port: 8080,
+    proxy: {
+      '/': 'http://127.0.0.1:8000/'
+    }
   }
 })

No other changes (webpack upgrade, submodule, tests…) have to be made, and we'll avoid CI breakages as well.

@phanan
Copy link
Member

phanan commented Apr 17, 2020

Can you allow project collaborators (me 😀) to push to your branches (both koel/koel and koel/core)?

@Hyzual
Copy link
Contributor Author

Hyzual commented Apr 18, 2020

Hi, yes I'm fine with keeping it minimal. Upgrading webpack / laravel-mix should be done nonetheless but can be another PR.
I'm not sure how to proceed. Should I revert some previous changes like the submodule update and the package.json ?

The "Allow edits from maintainers." checkbox is checked so you should be able to push to the branch. In case it's not enough I've invited you to the fork repo :)
To be honest I don't understand why the CI seems to never stop and goes into timeout on cypress :/

@phanan
Copy link
Member

phanan commented Apr 18, 2020

Thanks, I've pushed my changes. Let's see how it goes :)

@phanan phanan changed the title Upgrade webpack to latest Fix hotloading Apr 18, 2020
@phanan phanan merged commit d841ca7 into koel:master Apr 18, 2020
@phanan phanan deleted the webpack_build branch April 18, 2020 10:15
@phanan
Copy link
Member

phanan commented Apr 18, 2020

Thanks!

javier-lopez pushed a commit to javier-lopez/koel that referenced this pull request Aug 5, 2020
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.

2 participants