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

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

Merged
merged 5 commits into from Jan 25, 2019

Conversation

Projects
None yet
7 participants
@Naerriel
Copy link
Contributor

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.

Naerriel added some commits Jan 23, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 24, 2019

Any linux users to review this, @nosolosw maybe?

@nosolosw nosolosw self-requested a review Jan 24, 2019

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Jan 24, 2019

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

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Jan 24, 2019

cc @notnownikki as well

@notnownikki

This comment has been minimized.

Copy link
Member

notnownikki commented Jan 24, 2019

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

Yes, works. Lovely, delightful, wonderful!

@nosolosw nosolosw added this to the 5.0 (Gutenberg) milestone Jan 25, 2019

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw 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.

@nosolosw nosolosw requested review from WordPress/gutenberg and removed request for nosolosw Jan 25, 2019

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 25, 2019

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

@aduth
Copy link
Member

aduth left a comment

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

(Related: #13462)

Show resolved Hide resolved package.json Outdated
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 25, 2019

Tested macOS:

image

Tested Windows 10:

image

@Naerriel

This comment has been minimized.

Copy link
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

This comment has been minimized.

Copy link
Member

aduth commented Jan 25, 2019

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

Copy link
Member

aduth left a comment

Thanks, nice improvement 👍

@Naerriel

This comment has been minimized.

Copy link
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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@notnownikki

This comment has been minimized.

Copy link
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

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…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)
  ...
@Rahmon

This comment has been minimized.

Copy link
Contributor

Rahmon commented Jan 31, 2019

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

@Naerriel

This comment has been minimized.

Copy link
Contributor Author

Naerriel commented Jan 31, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment