Add LiveReload #663

amyrlam · 2018-05-15T08:17:00Z

Motivation

Resolves: #234

Have you read the Contributing Guidelines on pull requests?

yes!

Test Plan

LiveReload now works on save. LiveReload is watching docs/, blog/, pages/ and static/.

I QA'd the process.env.NODE_ENV === 'production' case locally. In website/package.json, I changed yarn start to "start": "NODE_ENV=production node ../lib/start-server.js". I then confirmed with a fresh yarn start that LiveReload wasn't running in the terminal, or present in the browser.

Related PRs

Updated the docs admin/testing-changes-on-Docusaurus-itself.md in this PR.

cc @yangshun please let me know if any feedback, can make edits!

docusaurus-bot · 2018-05-15T08:18:22Z

Deploy preview for docusaurus-preview ready!

Built with commit 5584173

https://deploy-preview-663--docusaurus-preview.netlify.com

JoelMarcey · 2018-05-15T14:38:23Z

😮 👍 ❤️ 🎉

I wake up to see this! This is so exciting!

yangshun

Excited about the PR! I'll let @JoelMarcey have a final look through before merging.

yangshun · 2018-05-16T04:48:43Z

lib/core/Site.js

@@ -159,6 +159,9 @@ class Site extends React.Component {
                }}
              />
            ))}
+          {process.env.NODE_ENV !== 'production' && (


Instead of checking whether it's production, check whether it's development.

Ok 👍. I went with it this way cause when debugging locally I did a console.log of process.env.NODE_ENV and it was undefined. But could be development or !process.env.NODE_ENV?

That's because it's not being set yet. I set it programmatically in server.js so that the user does not have to do anything in addition.

(To clarify, still confused why it was undefined locally...)

ohh I see, thanks for the clarification 👍. Sorry typed my latest before refreshing the page to see your comment about it not being set.

Oh, process.env looks at your process environmental variables. To set it outside of your code, you could do:

$ NODE_ENV=development npm run start

That would require users to have to change the way they start the server, which isn't ideal. I was thinking of passing in a prop developmentMode to all the top-level components like Site, BlogLayout, and read from it when determining whether to inject LiveReload. But there are too many places to add so I don't think it's a good idea to do so. Hence we set the variable in server/server.js.

yangshun · 2018-05-16T04:49:29Z

lib/core/Site.js

@@ -159,6 +159,9 @@ class Site extends React.Component {
                }}
              />
            ))}
+          {process.env.NODE_ENV !== 'production' && (
+            <script src="http://localhost:35729/livereload.js" />


This port number needs to be consistent with the server configuration. Hence extract the port out into a constant.

👍thanks, this is a good improvement, makes sense

yangshun · 2018-05-16T04:49:51Z

lib/server/server.js

+
+    // Gaze watches some specified files and triggers a callback when they change.
+    gaze(
+      ['../docs/**/*', 'blog/**/*', 'pages/**/*', 'static/**/*'],


I think it would be good to listen on the entire website dir.

Add to docs/, website/blog/, website/pages/, and website/static

amyrlam · 2018-05-16T09:03:06Z

lib/server/server.js

+  gaze(
+    [
+      '../docs/**/*', // docs
+      './**/*', // website


Couldn't quite grok why the dot syntax doesn't work in https://github.com/shama/gaze and minimatch, especially isaacs/minimatch#30. (LiveReload doesn't "fire up" in the terminal like the gif with this syntax here.)

Switched it to just star syntax in the latest commit. Let me know what you think...

Ok let's switch it to **/*

amyrlam · 2018-05-16T09:07:11Z

lib/server/server.js

@@ -559,7 +559,7 @@ function execute(port) {
  gaze(
    [
      '../docs/**/*', // docs
-      './**/*', // website
+      '*/*.*', // website


On second thought, maybe this approach is flawed re: https://stackoverflow.com/a/21835063/2628398. If some file was in root of website it wouldn't be watched.

Off to sleep, will revisit this tomorrow...

One last thing, it's confusing that **/*.* doesn't work...

.* specifies any extension and is redundant because * would do the same (in most cases).

If some file was in root of website it wouldn't be watched.

Correct. */*.* only looks at the files directly website/<dir>, files which are deeper such as pages/en/index.js would not be watched.

One last thing, it's confusing that **/. doesn't work...

**/*.* works for me.

I think **/* is the safest and is what we need. What's the rationale for changing to */*.*?

Sorry for the confusion!

With the ./**/* syntax there LiveReload doesn't start up. It seems to be because of the starting .. It hangs like the gif at the end.

HEAD is now at 11bf350 Tweak code a little alam@tesla website ((HEAD detached at 11bf350)) $ yarn start yarn run v1.6.0 warning package.json: No license field $ node ../lib/start-server.js

Delete the .* syntax locally and everything is fine:

alam@tesla website (amy/add-livereload-final) $ git co 2c264e898efda48ccd6bca9ad7e5f8225fce1c82 Note: checking out '2c264e898efda48ccd6bca9ad7e5f8225fce1c82'. You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by performing another checkout. If you want to create a new branch to retain commits you create, you may do so (now or later) by using -b with the checkout command again. Example: git checkout -b <new-branch-name> HEAD is now at 2c264e8 Simplify process.env call alam@tesla website ((HEAD detached at 2c264e8)) $ yarn start yarn run v1.6.0 warning package.json: No license field $ node ../lib/start-server.js Starting Docusaurus server on http://localhost:3000 Livereload listening on 35729...

So now how to fix watching all of website/? Locally, change to **/*.*. It also doesn't work.

Even delete docs (which is wrong) and just [ '**/*', // website ], doesn't run, like the gif?

Something else I'm missing? 🤔

That's super odd. Is working fine for me. Could you reinstall the dependencies or at worst, fetch a clean copy of the repository?

Change it back to **/* for now. I'm pretty confident that's the right path to specify.

yangshun

The path for recursive files under website is now incorrect IMO. @amyrlam could you verify?

amyrlam · 2018-05-16T17:39:29Z

I changed it to `**/*. But it still doesn't work for me, same behavior as the gif in the thread.

I rm -rf'd the directory, re-cloned and did a yarn install in root and website. master works fine for me locally.

Stumped rn re: what's wrong with my local setup

yangshun · 2018-05-16T17:53:20Z

@JoelMarcey could you help give it a go locally?

JoelMarcey · 2018-05-17T00:13:22Z

Let me check this out - I wonder if the local package-lock.json is clashing with the one on GitHub or something.

JoelMarcey · 2018-05-17T00:19:00Z

It looks like I got it running. I see the site locally and this at the command line.

But there was big change to package-lock.json locally after I ran npm install. And a smaller change to package.json

I wonder if the React 16 move, is causing trouble.

JoelMarcey · 2018-05-17T00:20:05Z

I can push an update to package-lock.json via a PR and then we can rebase this on that change to see if that makes a difference for @amyrlam ?

amyrlam · 2018-05-17T00:30:10Z

@JoelMarcey sure, I can also debug my issue separately from this PR, whatever is easier...

Hm I've been using yarn install. Just noticed there is a package-lock.json and a yarn.lock in Docusaurus? I only committed the yarn.lock here: https://github.com/facebook/Docusaurus/pull/665/files

JoelMarcey · 2018-05-17T00:34:03Z

We have had both. (whether we need both, that may be another question?). And you also changed yarn.lock in #665 as well.

My initial feeling here is that there is some conflict going on with you locally. But I am not 100% sure yet.

amyrlam · 2018-05-17T00:49:00Z

I have it working again finally! 😅

I "nombom'd" and blew away node_modules in root and website/, redid npm installs, which basically brought in your change to the package-lock.json in #669.

I'm not quite sure what happened, but I'm seeing reliable results with npm now.

yangshun · 2018-05-17T02:53:32Z

Tested on another MacBook of mine and it works. @JoelMarcey I think it's good to ship. Would be great if you got to try it out too!

JoelMarcey · 2018-05-17T14:23:59Z

@amyrlam @yangshun I tested on my machine and it works! I tested changing some docs, some pages (e.g. help.js). I can even change some things in siteConfig.js -- like images.

This is great!

The only things that didn't live reload were some changes to siteConfig.js. e.g., headerLinks and colors primaryColor, etc. Those don't work for you, do they?

Not sure we can do much about that, but it could be something we try to update later. The colors would probably be most useful to live reload, I think.

endiliey · 2018-05-17T17:51:11Z

Hi, sorry if I interfered in this discussion. This is awesome. Waiting this to be shipped. I've tried it locally and it works
But there are some part that didn't work, especially colors in siteConfig.js like what @JoelMarcey mentioned. I agree that colors would probably be most useful to live reload

Human are curious by nature, so I digged a little bit to see what's wrong. Modified amy's lib/server/server.js gaze part to debug

// Listen for all kinds of file changes - modified/added/deleted.
this.on('all', function(event, filepath) {
   console.log(filepath + ' was ' + event);

Then, I went to edit primaryColor in siteconfig.js

LiveReload happens but the css is not changed. I had to yarn start again

Notice that main.css is not re-built on reload. It's only built once on yarn start.
Relevant codes:
https://github.com/amyrlam/Docusaurus/blob/amy/add-livereload-final/lib/server/server.js#L459-L514

Maybe we could trigger the css generation & sent it to user if siteConfig.js is changed

Hopefully this help !

yangshun · 2018-05-17T17:59:55Z

siteConfig will not be reloaded because it's require()-ed during initialization and not on each page load. I think it would require a huge change to support livereload on config changes. It's not typical for processes to reload config after the initial start up. For reference, webpack, Flow and Jest also do not reload the config after the server starts running.

I'm aware of this limitation in this PR and I think it's fine to ship as-is. The part about supporting siteConfig can be found in this task #267

endiliey · 2018-05-17T18:02:36Z

That makes sense. Awesome

JoelMarcey · 2018-05-17T18:03:32Z

I 100% agree that we can ship this as-is.

I will say one thing -- I changed the image path in siteConfig.js and it picked something up, because the image changed when I did it. Maybe because the server still looks in siteConfig.js directly for path information?

amyrlam · 2018-05-17T18:04:05Z

@endiliey not interfering at all, appreciate you chiming in! 👍

Yes I'm seeing the same thing as @JoelMarcey re: colors. I'm not sure why this is happening yet, but probably that the colors are only loaded on start.

JoelMarcey

❤️ 😍

facebook-github-bot added the CLA Signed Signed Facebook CLA label May 15, 2018

JoelMarcey requested review from yangshun and JoelMarcey May 15, 2018 14:38

yangshun approved these changes May 16, 2018

View reviewed changes

amyrlam force-pushed the amy/add-livereload-final branch from 1d99b14 to c44590e Compare May 16, 2018 07:48

amyrlam and others added 7 commits May 16, 2018 00:59

Add LiveReload

a6b1020

Add to docs/, website/blog/, website/pages/, and website/static

Only run LiveReload in development

7ff7b27

Prettify file

e9ecf52

Add catch for NODE_ENV being undefined

8127dfd

Add LiveReload docs update

b63db94

Simplify process.env call

2c264e8

Tweak code a little

11bf350

amyrlam force-pushed the amy/add-livereload-final branch from c44590e to 11bf350 Compare May 16, 2018 07:59

Fix LiveReload path

e703dde

amyrlam commented May 16, 2018

View reviewed changes

Forgot to change the LiveReload port back

7c5bb0d

yangshun suggested changes May 16, 2018

View reviewed changes

Fix gaze

04f627d

JoelMarcey mentioned this pull request May 17, 2018

Update package-lock.json #669

Merged

yangshun approved these changes May 17, 2018

View reviewed changes

amyrlam and others added 2 commits May 16, 2018 22:52

Merge branch 'master' into amy/add-livereload-final

37142b8

Update package-lock.json with required entities for Live Reload

8f6377f

Merge branch 'master' into amy/add-livereload-final

5584173

JoelMarcey approved these changes May 17, 2018

View reviewed changes

JoelMarcey merged commit f9a0907 into facebook:master May 17, 2018

amyrlam deleted the amy/add-livereload-final branch October 2, 2018 05:29

Add LiveReload #663

Add LiveReload #663

Conversation

amyrlam commented May 15, 2018

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

docusaurus-bot commented May 15, 2018 • edited Loading

JoelMarcey commented May 15, 2018

yangshun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangshun May 16, 2018 • edited Loading

Choose a reason for hiding this comment

amyrlam May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangshun left a comment

Choose a reason for hiding this comment

amyrlam commented May 16, 2018

yangshun commented May 16, 2018

JoelMarcey commented May 17, 2018

JoelMarcey commented May 17, 2018

JoelMarcey commented May 17, 2018

amyrlam commented May 17, 2018

JoelMarcey commented May 17, 2018

amyrlam commented May 17, 2018

yangshun commented May 17, 2018

JoelMarcey commented May 17, 2018

endiliey commented May 17, 2018 • edited Loading

yangshun commented May 17, 2018 • edited Loading

endiliey commented May 17, 2018

JoelMarcey commented May 17, 2018

amyrlam commented May 17, 2018

JoelMarcey left a comment

Choose a reason for hiding this comment

docusaurus-bot commented May 15, 2018 •

edited

Loading

yangshun May 16, 2018 •

edited

Loading

amyrlam May 16, 2018 •

edited

Loading

endiliey commented May 17, 2018 •

edited

Loading

yangshun commented May 17, 2018 •

edited

Loading