Skip to content

Add watcher on Linux: change fs to node-watch#13448

Merged
aduth merged 5 commits intoWordPress:masterfrom
Naerriel:try/watcher-on-linux
Jan 25, 2019
Merged

Add watcher on Linux: change fs to node-watch#13448
aduth merged 5 commits intoWordPress:masterfrom
Naerriel:try/watcher-on-linux

Conversation

@Naerriel
Copy link
Copy Markdown
Contributor

@Naerriel Naerriel commented Jan 23, 2019

Description

Fix to #13140.
It allows watcher to detect changes in files on Linux OS, as recursive option in fs.watch isn't supported on Linux.
It adds a new developer dependency: node-watch.

How has this been tested?

I modified a file and the watcher recognized it and built the project.

My testing environment is Ubuntu 16.04.4

Screenshots

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@youknowriad
Copy link
Copy Markdown
Contributor

youknowriad commented Jan 24, 2019

Any linux users to review this, @nosolosw maybe?

@oandregal oandregal self-requested a review January 24, 2019 11:04
@oandregal
Copy link
Copy Markdown
Member

oandregal commented Jan 24, 2019

Adding myself as a reviewer, will come to this later in the day.

@oandregal
Copy link
Copy Markdown
Member

oandregal commented Jan 24, 2019

cc @notnownikki as well

@notnownikki
Copy link
Copy Markdown
Member

notnownikki commented Jan 24, 2019

😀 😀 😀 I no longer have to rebuild everything from scratch when making changes! 👯‍♀️

Yes, works. Lovely, delightful, wonderful!

@oandregal oandregal added the [Type] Build Tooling Issues or PRs related to build tooling label Jan 25, 2019
@oandregal oandregal added this to the 5.0 (Gutenberg) milestone Jan 25, 2019
@oandregal
Copy link
Copy Markdown
Member

oandregal commented Jan 25, 2019

@youknowriad added this to the 5.0 milestone so it doesn't get lost. Before approval, next step would be that someone tests this on OSX.

@oandregal oandregal requested review from a team and removed request for oandregal January 25, 2019 10:29
@gziolo
Copy link
Copy Markdown
Member

gziolo commented Jan 25, 2019

It would be nice to test with Windows, too :)

aduth
aduth requested changes Jan 25, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@aduth aduth left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you re-run npm install with the latest version of npm (v6.7.0) ? For me, many changes are introduced.

(Related: #13462)

@aduth
Copy link
Copy Markdown
Member

aduth commented Jan 25, 2019

Tested macOS:

image

Tested Windows 10:

image

@Naerriel
Copy link
Copy Markdown
Contributor Author

Naerriel commented Jan 25, 2019

@aduth I've ran into a trouble regarding auto addition of "optional": true parameters to package-lock.json which happens probably because I'm on Linux and not on Mac but now everything should be in order. Can you check it, please?

@aduth
Copy link
Copy Markdown
Member

aduth commented Jan 25, 2019 •
edited
Loading

Ah yes, there are some platform-specific issues with npm on Linux, where optional flags will differ.

See: npm/npm#17722

I've pushed up the resulting changes I see on my end with npm install in e6b1a22.

aduth
aduth approved these changes Jan 25, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice improvement 👍

@Naerriel
Copy link
Copy Markdown
Contributor Author

Naerriel commented Jan 25, 2019

Thank you. Yes, that was my original code but I deleted the optional flags manually, thinking they are unintended.

Thank you 😄

@aduth aduth merged commit 1748145 into WordPress:master Jan 25, 2019
@notnownikki
Copy link
Copy Markdown
Member

notnownikki commented Jan 25, 2019

@Naerriel and @aduth you're my heroes of the month!

daniloercoli added a commit that referenced this pull request Jan 26, 2019
@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
92cc888 
…rnmobile/372-enter-key-detection-to-title

* 'master' of https://github.com/WordPress/gutenberg: (29 commits)
  Update for RangeControl documentation (#12564)
  Plugin: Deprecate gutenberg_load_list_reusable_blocks (#13456)
  Update the columns attribute in onSelectImages so that if images are removed via the media modal, the columns can't be higher than the new number of images (#13488)
  Replace the fullscreen "exit" icon with a back arrow (#13403)
  Include :visited links in button color (#12183)
  Amazon Kindle block (#13510)
  Plugin: Deprecate gutenberg_prepare_blocks_for_js (#13457)
  Add watcher on Linux: change fs to node-watch (#13448)
  Plugin: Deprecate `gutenberg` theme support (#13458)
  Datepicker: Add inValidDay support (#12962)
  Block Switcher: Render disabled button even if multi-selection (#13431)
  Plugin: Deprecate gutenberg_register_post_types (#13468)
  Plugin: Deprecate register_tinymce_scripts (#13466)
  Set minimum of words for RSS excerpt (#13502)
  Plugin: Deprecate gutenberg_get_block_categories (#13454)
  Plugin: Deprecate gutenberg_content_block_version (#13469)
  API Fetch: Expose nonce on created middleware function (#13451)
  Plugin: Remove list screens integrations (#13459)
  Plugin: Remove core-defined block detection functions (#13467)
  Spec Parser: Move generated spec parser to package (#13493)
  ...
@gziolo gziolo mentioned this pull request Jan 31, 2019
Closed
@Rahmon
Copy link
Copy Markdown
Contributor

Rahmon commented Jan 31, 2019

Thanks for fix. I've spent hours to find out the problem.

@Naerriel
Copy link
Copy Markdown
Contributor Author

Naerriel commented Jan 31, 2019

You're welcome. It would be impossible if you haven't diagnosed it first 😄

This was referenced Feb 4, 2019
Merged
Closed
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 6, 2019
Merged
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@Naerriel @youknowriad
Add watcher on Linux: change fs to node-watch (#13448)
2a84fa3 
* Add watcher on Linux: change fs to node-watch

* Add watcher on Linux: Change fs to node-watch (pt.2.)

* Add watcher on Linux: Change fs to node-watch (pt.3.)

* Framework: Regenerate package-lock.json
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@Naerriel @youknowriad
Add watcher on Linux: change fs to node-watch (#13448)
d02b6e9 
* Add watcher on Linux: change fs to node-watch

* Add watcher on Linux: Change fs to node-watch (pt.2.)

* Add watcher on Linux: Change fs to node-watch (pt.3.)

* Framework: Regenerate package-lock.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduth aduth aduth approved these changes

Assignees

No one assigned

Labels

[Type] Build Tooling Issues or PRs related to build tooling

Projects

None yet

Milestone

5.0 (Gutenberg)

Development

Successfully merging this pull request may close these issues.

7 participants

@Naerriel @youknowriad @oandregal @notnownikki @gziolo @aduth @Rahmon