newhavenio / newhavenio.github.io

active version of the website for newhaven.io built on the Jekyll framework
http://newhavenio.github.io/
MIT License
13 stars 12 forks source link

Run webpack and jekyll together #100

Closed maxx1128 closed 6 years ago

maxx1128 commented 6 years ago

I've started the redesign, and noticed the workflow isn't very good for editing and seeing the results. The biggest issue is that you need to run the webpack command whenever you want to see a CSS or Sass change.

I adjusted the config so you can just run one command for both webpack and jekyll. They'll both watch for changes in the pages, styles, or functionality, and reload the affected files as needed. People who want to run the site locally will only need yarn start. This lowers the entry barrier for people who want to make changes and confirm they work before submitting a PR.

jnimety commented 6 years ago

You can use &&, no? On Tue, May 15, 2018 at 3:42 PM Devin Weaver notifications@github.com wrote:

@sukima commented on this pull request.

In package.json https://github.com/newhavenio/newhavenio.github.io/pull/100#discussion_r188412543 :

@@ -37,6 +37,7 @@ "webpack": "^3.11.0" }, "scripts": {

  • "build": "webpack"
  • "start": "jekyll serve --incremental --watch | webpack -w",

Looking into this I think the pipe is actually not correct. It seems the act of piping may have the side effect of working as expected but it isn't intended. webpack does have a --watch-stdin I think for this purpose however, after googling for CLI documentation on webpack (hint: there seem to be none) I found this blog post that is doing exactly what your looking for:

https://www.jonathan-petitcolas.com/2016/08/12/plugging-webpack-to-jekyll-powered-pages.html

And they use a program called concurrency which is installed via npm (yarn in our case). I think that looks like a much cleaner solution then to rely on shell file handler mechanics to simulate job control mechanics.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/newhavenio/newhavenio.github.io/pull/100#discussion_r188412543, or mute the thread https://github.com/notifications/unsubscribe-auth/AAETVASOgv1OXL0mfLxjOEFmTTmBTBQrks5tyy-pgaJpZM4T_7TC .

jnimety commented 6 years ago

Or to you want to run both in “watch” mode? On Tue, May 15, 2018 at 3:45 PM Joel Nimety jnimety@gmail.com wrote:

You can use &&, no? On Tue, May 15, 2018 at 3:42 PM Devin Weaver notifications@github.com wrote:

@sukima commented on this pull request.

In package.json https://github.com/newhavenio/newhavenio.github.io/pull/100#discussion_r188412543 :

@@ -37,6 +37,7 @@ "webpack": "^3.11.0" }, "scripts": {

  • "build": "webpack"
  • "start": "jekyll serve --incremental --watch | webpack -w",

Looking into this I think the pipe is actually not correct. It seems the act of piping may have the side effect of working as expected but it isn't intended. webpack does have a --watch-stdin I think for this purpose however, after googling for CLI documentation on webpack (hint: there seem to be none) I found this blog post that is doing exactly what your looking for:

https://www.jonathan-petitcolas.com/2016/08/12/plugging-webpack-to-jekyll-powered-pages.html

And they use a program called concurrency which is installed via npm (yarn in our case). I think that looks like a much cleaner solution then to rely on shell file handler mechanics to simulate job control mechanics.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/newhavenio/newhavenio.github.io/pull/100#discussion_r188412543, or mute the thread https://github.com/notifications/unsubscribe-auth/AAETVASOgv1OXL0mfLxjOEFmTTmBTBQrks5tyy-pgaJpZM4T_7TC .

maxx1128 commented 6 years ago

@jnimety Both are running in watch mode, but the most important thing is having them both run concurrently. If && works that'd be the easiest fix, if it doesn't mean adding on anything extra. I'll give it a try.

jnimety commented 6 years ago

&& won’t give concurrency (I don’t think). Let me look up shell syntax to do what you want. ‘tee’ might be appropriate here but I need to check. On Tue, May 15, 2018 at 3:47 PM Max Antonucci notifications@github.com wrote:

@jnimety https://github.com/jnimety Both are running in watch mode, but the most important thing is having them both run concurrently. If && works that'd be the easiest fix, if it doesn't mean adding on anything extra. I'll give it a try.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/newhavenio/newhavenio.github.io/pull/100#issuecomment-389290795, or mute the thread https://github.com/notifications/unsubscribe-auth/AAETVM9THHfwqjyC07l9GnFdUS_xsfeDks5tyzDdgaJpZM4T_7TC .

jnimety commented 6 years ago

‘command1 & command2 &’ will run both in the background in parallel On Tue, May 15, 2018 at 3:48 PM Joel Nimety jnimety@gmail.com wrote:

&& won’t give concurrency (I don’t think). Let me look up shell syntax to do what you want. ‘tee’ might be appropriate here but I need to check. On Tue, May 15, 2018 at 3:47 PM Max Antonucci notifications@github.com wrote:

@jnimety https://github.com/jnimety Both are running in watch mode, but the most important thing is having them both run concurrently. If && works that'd be the easiest fix, if it doesn't mean adding on anything extra. I'll give it a try.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/newhavenio/newhavenio.github.io/pull/100#issuecomment-389290795, or mute the thread https://github.com/notifications/unsubscribe-auth/AAETVM9THHfwqjyC07l9GnFdUS_xsfeDks5tyzDdgaJpZM4T_7TC .

jnimety commented 6 years ago

If the ‘parallel’ command is available we can also use that. On Tue, May 15, 2018 at 3:52 PM Joel Nimety jnimety@gmail.com wrote:

‘command1 & command2 &’ will run both in the background in parallel On Tue, May 15, 2018 at 3:48 PM Joel Nimety jnimety@gmail.com wrote:

&& won’t give concurrency (I don’t think). Let me look up shell syntax to do what you want. ‘tee’ might be appropriate here but I need to check. On Tue, May 15, 2018 at 3:47 PM Max Antonucci notifications@github.com wrote:

@jnimety https://github.com/jnimety Both are running in watch mode, but the most important thing is having them both run concurrently. If && works that'd be the easiest fix, if it doesn't mean adding on anything extra. I'll give it a try.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/newhavenio/newhavenio.github.io/pull/100#issuecomment-389290795, or mute the thread https://github.com/notifications/unsubscribe-auth/AAETVM9THHfwqjyC07l9GnFdUS_xsfeDks5tyzDdgaJpZM4T_7TC .

maxx1128 commented 6 years ago

‘command1 & command2 &’ works for me, I'll amend it to this

sukima commented 6 years ago

Please understand using the background & inside a script like that i think could lead to zombie processes. If anything it is odd and likely not portable. These reasons is why I did not suggest them.

Instead I suggested the concurrently node module which is easy to add to the devDependencies, available in scripts, and designed specifically for this use case. The linked blog post describes this in detail.

sukima commented 6 years ago

I don't believe npm is the best tool to manage complex relationships between processes.

You would be far better served by creating a node script that uses node's child_process module to manage the launching and killing of co-processes, perhaps using spawn.

(https://stackoverflow.com/a/27809215/227176)

The concurrently module does exactly that :point_up:

sukima commented 6 years ago

An alternative would be to write a really small shell script and calling it from npm scripts. That would offer proper job management.

treznick commented 6 years ago

@sukima @max1128 I'll let you both figure this one out :)

On Tue, May 15, 2018 at 6:30 PM Devin Weaver notifications@github.com wrote:

An alternative would be to write a really small shell script and calling it from npm scripts. That would offer proper job management.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/newhavenio/newhavenio.github.io/pull/100#issuecomment-389334089, or mute the thread https://github.com/notifications/unsubscribe-auth/AAlwAzcjjxQwRAieOXtv5I9VmTsOUGhnks5ty1cIgaJpZM4T_7TC .

bsutt123 commented 6 years ago

Can confirm that while & works you better be ready to use fuser to find zombie processes with ctrl-c doesn't quite work.

EDIT: It isn't the end of the world, but it does create a barrier to entry for new users who might not know how to find and kill processes from the command line.

I also think that while it is nice to have a single command, it isn't the worst to have to have 2 terminals open and run each command separately. Its honestly what i usually do because it lets me restart one and not the other.

sukima commented 6 years ago

@bsutt123 can you test this after my latest commit ddafa74 where it uses concurrently? In my testing the zombie process problem never happened with the latest commit.

maxx1128 commented 6 years ago

@sukima @bsutt123 Sorry for the delay in testing this, was having some bundler issues. But it works great for me so I'm all for this solution.