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

Build: Packages: Ignore non-JS file events #7697

Merged
merged 1 commit into from Jul 4, 2018

Conversation

Projects
None yet
3 participants
@mcsf
Contributor

mcsf commented Jul 3, 2018

Description

Editors (e.g. Vim, gedit) typically rely on temporary swap files to manage a user's editing session. When editing a file, e.g.

packages/core-data/src/actions.js

a temporary file may be created:

packages/core-data/src/.actions.js.swp // or
packages/core-data/src/actions.js~

This PR checks for filename format to prevent most scenarios; essentially, files should end with .js and not start with . (an indicator, in Unix tradition, of hidden files).

Without this guard, upon creation of a swap file, the watcher will attempt to build from the swap file, resulting in syntax errors.

How has this been tested?

  • Run npm run dev and wait for the initial build
  • Use Vim with default swapfile settings (and related swapsync, etc.)
  • Open a file in packages/, e.g. packages/core-data/src/actions.js
  • Start editing it, without saving
  • Confirm that a swapfile is being used in the file's directory: :swapname
  • Confirm that no rebuild is triggered as you edit without saving
  • Save the open file
  • Confirm that a rebuild is triggered
  • Close the saved file
  • Confirm that no rebuild is triggered

Screenshots

screen shot 2018-06-28 at 15 38 05

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@mcsf mcsf added this to the 3.2 milestone Jul 3, 2018

@mcsf mcsf requested a review from gziolo Jul 3, 2018

// Exclude deceitful source-like files, such as editor swap files.
const isSourceFile = ( filename ) => {
return filename.indexOf( '.' ) !== 0 && filename.match( /\.js$/ );

This comment has been minimized.

@aduth

aduth Jul 3, 2018

Member

Minor: Generally recommend using RegExp#test if you only care about boolean value of match, as aside from being more semantically correct, there can tend to be a performance difference:

https://jsperf.com/test-vs-match-regex

Also wonder if this could be simplified to just the regular expression, skipping the String#indexOf:

return /.\.js$/.test( filename );

But I'm wondering: What are we excluding with that condition? Isn't this a full URL path? This isn't entirely clear by the comment.

This comment has been minimized.

@mcsf

mcsf Jul 3, 2018

Contributor

I agree with both: I was thinking I should use test, but match just stuck; targeting a leading . was my first reflex to exclude "hidden files", but I think we're alright with just /.\.js$/.

This comment has been minimized.

@mcsf

mcsf Jul 3, 2018

Contributor

Amended in 2559cff

Build: Packages: Ignore non-JS file events
Editors (e.g. Vim, gedit) typically rely on temporary swap files to
manage a user's editing session. When editing a file, e.g.

  packages/core-data/src/actions.js

a temporary file may be created:

  packages/core-data/src/.actions.js.swp, or
  packages/core-data/src/actions.js~

This commit checks for filename format to prevent such scenarios;
essentially, files should end with `.js`.

Without this guard, upon creation of a swap file, the watcher will
attempt to build from the swap file, resulting in syntax errors.
@gziolo

This comment has been minimized.

Member

gziolo commented Jul 4, 2018

At some point, we will have to handle also .pegjs, .css and .scss files. In case of Babel only .js makes sense as you proposed.

@gziolo

gziolo approved these changes Jul 4, 2018

It works as advertised. I think we will have to relax this check pretty soon as noted in my previous comment.

@mcsf mcsf merged commit 375b406 into master Jul 4, 2018

2 checks passed

codecov/project 46.61% remains the same compared to bf09112
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mcsf mcsf deleted the fix/build-packages-ignore-swapfiles branch Jul 4, 2018

@mcsf

This comment has been minimized.

Contributor

mcsf commented Jul 4, 2018

Thanks!

I think we will have to relax this check pretty soon as noted in my previous comment.

👍

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