Skip to content
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 Tooling: Pass individual files as arguments from watch to build script #15219

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 26, 2019

Fixes #14960

This pull request seeks to resolve an error which can occur while running npm run dev, often when switching between branches (Error: ENAMETOOLONG: name too long).

The underlying cause is that when passing files as arguments to build.js, it expects many arguments:

const files = process.argv.slice( 2 );

...but in fact, the watch script would have only passed at most one, a single argument of space-separated changed files:

execSync( `${ BUILD_CMD } \"${ files.join( ' ' ) }\"`, { stdio: [ 0, 1, 2 ] } );

The changes here address this by assuring that each file is passed as its own argument, made simpler by substituting exec with spawn, which accepts args as part of its function signature.

However, while this addresses the immediate issue, testing the effects had me begging the question: This was previously never working correctly to rebuild files, and it's unclear why we'd want to watch them at all. Notably, the files being built here are the ones generated to the build directory of the individual packages, which is relevant for NPM publishing, but not so much for use by the plugin. Separately, we may want to consider whether wp-scripts start should be the sole command of npm run dev.

Testing Instructions:

With this branch checked out, start npm run dev. Then, switch between branches where watched files would be changed (e.g. between master and try/scroll=beyond-end) and verify that the [1] -> update: entries are shown correctly, and no errors occur.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling labels Apr 26, 2019
@aduth aduth requested review from youknowriad and ellatrix April 26, 2019 17:09
@aduth
Copy link
Member Author

aduth commented Apr 26, 2019

However, while this addresses the immediate issue, testing the effects had me begging the question: This was previously never working correctly to rebuild files, and it's unclear why we'd want to watch them at all. Notably, the files being built here are the ones generated to the build directory of the individual packages, which is relevant for NPM publishing, but not so much for use by the plugin. Separately, we may want to consider whether wp-scripts start should be the sole command of npm run dev.

I took a look back to #6708 and it makes more sense now why it's done this way:

The idea is to consider these packages as real external modules. Building happens in two steps, the build script generates the build and build-module transpiled files and webpack uses these built files to generate its bundle with the library support (global variable).

i.e. the build.js script will produce the package distributables. Webpack will detect changes in those files to trigger its own rebuild.

It still seems like there's some redundancy here in that we could either (a) just let Webpack compile from the source files or (b) avoid including Babel processing in the Webpack configuration, since it's already operating on the transpiled output.

@aduth
Copy link
Member Author

aduth commented Apr 26, 2019

It still seems like there's some redundancy here in that we could either (a) just let Webpack compile from the source files or (b) avoid including Babel processing in the Webpack configuration, since it's already operating on the transpiled output.

See #15226, which takes the latter approach.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It works as advertised. I can confirm that I couldn't reproduce the issue. Code changes look good 👍

@aduth aduth merged commit 1e96ec7 into master Apr 29, 2019
@aduth aduth deleted the fix/14960-watch-rebuild branch April 29, 2019 20:40
@ellatrix
Copy link
Member

ellatrix commented May 1, 2019

I'm now getting the following error every time I run npm test:

[1] Failed to read config file: /Users/ella/gutenberg/packages/block-editor/build/components/block-edit/index.js /Users/ella/gutenberg/packages/block-editor/src/components/block-list-appender/style.scss /Users/ella/gutenberg/packages/block-editor/src/components/button-block-appender/style.scss /Users/ella/gutenberg/packages/block-editor/src/components/rich-text/index.native.js /Users/ella/gutenberg/packages/block-editor/src/components/rich-text/style.scss /Users/ella/gutenberg/packages/block-library/src/group/edit.js /Users/ella/gutenberg/packages/block-library/src/heading/edit.native.js /Users/ella/gutenberg/packages/block-library/src/paragraph/edit.js /Users/ella/gutenberg/packages/block-library/src/paragraph/edit.native.js /Users/ella/gutenberg/packages/blocks/src/api/validation.js /Users/ella/gutenberg/packages/components/src/drop-zone/provider.js /Users/ella/gutenberg/packages/components/src/focal-point-picker/index.js /Users/ella/gutenberg/packages/data/src/components/with-dispatch/index.js /Users/ella/gutenberg/packages/date/package.json. 
[1] Error: ENAMETOOLONG: name too long, open '/Users/ella/gutenberg/packages/block-editor/build/components/block-edit/index.js /Users/ella/gutenberg/packages/block-editor/src/components/block-list-appender/style.scss /Users/ella/gutenberg/packages/block-editor/src/components/button-block-appender/style.scss /Users/ella/gutenberg/packages/block-editor/src/components/rich-text/index.native.js /Users/ella/gutenberg/packages/block-editor/src/components/rich-text/style.scss /Users/ella/gutenberg/packages/block-library/src/group/edit.js /Users/ella/gutenberg/packages/block-library/src/heading/edit.native.js /Users/ella/gutenberg/packages/block-library/src/paragraph/edit.js /Users/ella/gutenberg/packages/block-library/src/paragraph/edit.native.js /Users/ella/gutenberg/packages/blocks/src/api/validation.js /Users/ella/gutenberg/packages/components/src/drop-zone/provider.js /Users/ella/gutenberg/packages/components/src/focal-point-picker/index.js /Users/ella/gutenberg/packages/data/src/components/with-dispatch/index.js /Users/ella/gutenberg/packages/date/package.json'

Is it related to this?

@aduth
Copy link
Member Author

aduth commented May 1, 2019

@ellatrix I can't imagine it would have been caused by this, since the watch.js file shouldn't be relevant for npm test. But it could be of a similar sort of issue.

Tug added a commit that referenced this pull request May 2, 2019
commit 1dddab4
Author: Stefanos Togkoulidis <[email protected]>
Date:   Wed May 1 00:00:50 2019 +0300

    Have Aztec delete the detected Enter key for paragraphs

    Aztec-Android doesn't swallow the Enter key (like the list handling does) so,
    instruct Aztec to delete it for the paragraph block.

commit 0936ca0
Author: Stefanos Togkoulidis <[email protected]>
Date:   Tue Apr 30 20:58:36 2019 +0300

    Just use onFormatChange which now defaults to "force"

commit 6358de3
Merge: a5282ce 5e4d627
Author: Stefanos Togkoulidis <[email protected]>
Date:   Tue Apr 30 20:23:57 2019 +0300

    Merge branch 'master' into rnmobile/fix-list-handling-android

commit 5e4d627
Author: Pascal Birchler <[email protected]>
Date:   Tue Apr 30 18:27:24 2019 +0200

    Update to Babel 7.4 and core-js 3 (#15139)

    * Specify core-js version when using useBuiltIns

    * Update to Babel 7.4 and core-js 3

    * setupFiles: fix path to async-iterator

    * Also update Babel packages in babel-preset-default

    * Update snapshot for babel-preset-default test

    * Manually include web.dom-collections.iterator in jest config

    * Regenerate package-lock.json file after latest changes applied

    * Upgrade ESLint related npm packages

    * Another cleanup in the package-lock.json file

    * Remove ESLint rules which throw error to fix linting

    * Add core-js/modules/web.dom.iterable to make e2e tests work

    * Update Babel packages to the latest version

    * Update ignore files in e2e tests config

    * Add changelog entries related to Babel and ESLint version bumps

commit 1d959ba
Author: Andrés <[email protected]>
Date:   Tue Apr 30 17:14:35 2019 +0200

    Remove not used state (#15224)

commit d715e93
Author: Jon Surrell <[email protected]>
Date:   Tue Apr 30 16:29:09 2019 +0200

    Babel plugin JSX: Implement Fragment handling (#15120)

    Add imports for `<></>` JSX Fragments.

commit 96cad99
Author: Kelly Dwan <[email protected]>
Date:   Tue Apr 30 09:18:54 2019 -0400

    Fix incorrect ID in FocalPointPicker (#15255)

commit c1ba13e
Author: Jon Surrell <[email protected]>
Date:   Tue Apr 30 13:44:50 2019 +0200

    Update Fragment imports in READMEs (#15262)

    Some Fragment imports were incorrect. Fix them.

commit 297c2f4
Author: etoledom <[email protected]>
Date:   Tue Apr 30 13:19:34 2019 +0200

    RNMobile: Fix autoscroll on ListBlock (#15048)

    * RNMobile: Add ability to send extra props (i.e. props needed just for mobile) to the list block.

    * Adding onCaretVerticalPositionChange to RichText via context

    * Passing onCaretVerticalPositionChange via context to RichText for all RichText based blocks

    This makes it not necessary to pass onCaretVerticalPositionChange as a prop directly to RichText from the block component.

    * Update jest snapshot for block-edit

commit 447dd27
Author: Jorge Costa <[email protected]>
Date:   Tue Apr 30 10:47:55 2019 +0100

    Fix copy paste and delete error core paragraph with locking (#14712)

    * Fix: Copy / Paste error core/paragraph with locking

    * Add test case.

    * Update packages/block-library/src/paragraph/edit.js

    Co-Authored-By: jorgefilipecosta <[email protected]>

commit 0b2eb1c
Author: Jon Surrell <[email protected]>
Date:   Tue Apr 30 09:39:29 2019 +0200

    Dependency extraction webpack plugin: Change option name consistent with docs (#15260)

    In the docs, an option name was provided as `requestToHandle`, but the
    actual option in use was `requestToDependency`.

    Update the code and related tests to use `requestToHandle` as described
    in the documentation instead of `requestToDependency`.

commit 506827e
Author: Jon Surrell <[email protected]>
Date:   Tue Apr 30 09:31:38 2019 +0200

    Read script dependencies from generated files (#15124)

    * Rework script registration to use generated deps

    * Deprecate static list of dependencies

    * Rework translation without packages-dependencies.php

    * Include build/*/*.deps.json in plugin zip

    * Add script dependencies undetectable by build tools

    Some scripts have dependencies that are undetectable by the webpack
    plugin used to generate the dependency files. Add these dependencies
    to the generated dependencies.

    * Add `wp-polyfill` dependency via webpack plugin

    Enable `injectPolyfill` option in the webpack plugin instead of manually
    injecting the dependency via PHP on script registration.

    This will enable the polyfill for consumers of wp-scripts default
    webpack config.

    * Improve deprecated message

    * Remove obsolete lib/packages-dependencies.php

commit c2c8276
Author: Daniel Richards <[email protected]>
Date:   Tue Apr 30 13:36:49 2019 +0800

    Use button block appender on group block (#14943)

    * Use button block appender on group block

    * Update group block e2e tests for the new button appender

    * Add additional margin to the appender when a group has a background color.

    * Use more opaque versions of the appender background color

    To ensure greater compatibility when layered on top of a block with a background color.

    * Block Library: Display ButtonBlockAppender only if selected or empty

    * Only render button appender when the group has no inner blocks

commit 1e96ec7
Author: Andrew Duthie <[email protected]>
Date:   Mon Apr 29 16:40:24 2019 -0400

    Build Tooling: Pass individual files as arguments from watch to build script (#15219)

commit cafb57b
Author: Grzegorz (Greg) Ziółkowski <[email protected]>
Date:   Mon Apr 29 22:12:08 2019 +0200

    Blocks: Upgrade simple-html-tokenizer dependency (#15246)

commit 5b3b3ab
Author: Andrew Duthie <[email protected]>
Date:   Mon Apr 29 13:38:09 2019 -0400

    Framework: Add REST API codeowners (#15215)

commit 64c48fd
Author: Ian Dunn <[email protected]>
Date:   Mon Apr 29 10:06:46 2019 -0700

    Correct name of `date_i18n()` PHP function. (#15204)

commit 8c28814
Author: Andrew Duthie <[email protected]>
Date:   Mon Apr 29 13:05:10 2019 -0400

    Framework: Remove TESTS.md (#15217)

commit 0f1fb57
Author: Emmanuel Hesry <[email protected]>
Date:   Mon Apr 29 17:34:22 2019 +0200

    fix typo in withDispatch documentation (#15251)

commit a5282ce
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 16:08:31 2019 +0300

    Revert "Trivial change to trigger Travis"

    This reverts commit e22ffde.

commit e22ffde
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 16:08:24 2019 +0300

    Trivial change to trigger Travis

commit 15a51f8
Author: Jorge Costa <[email protected]>
Date:   Fri Apr 19 15:33:15 2019 +0100

    chore: Fix: Lint error that makes unit tests (and CI tests) fail. (#15073)

commit 2b11074
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 12:42:55 2019 +0300

    Fix lint issues

commit 9c298c8
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 12:01:46 2019 +0300

    Need to specify firedAfterTextChanged on all Aztec events

commit 46e086d
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 03:59:59 2019 +0300

    Force Aztec update if "Enter" fired before text change

commit 724e295
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 03:11:52 2019 +0300

    Differentiate Android and iOS since assumptions diverged

    The iOS side still expects to just check against `this.lastContent` to
    force the change into Aztec.

commit 52386fa
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 01:55:22 2019 +0300

    Use a flag to signal Aztec-originated changes

    And assume that when that flag is false, component changes need to get
    sent/reflected down to Aztec.

commit c7aa381
Author: Stefanos Togkoulidis <[email protected]>
Date:   Tue Apr 23 17:30:45 2019 +0300

    Able to not lose content

commit 60c75b0
Author: Stefanos Togkoulidis <[email protected]>
Date:   Tue Apr 23 10:09:23 2019 +0300

    If text already changed, don't modify it
Tug added a commit that referenced this pull request May 2, 2019
commit 1dddab4
Author: Stefanos Togkoulidis <[email protected]>
Date:   Wed May 1 00:00:50 2019 +0300

    Have Aztec delete the detected Enter key for paragraphs

    Aztec-Android doesn't swallow the Enter key (like the list handling does) so,
    instruct Aztec to delete it for the paragraph block.

commit 0936ca0
Author: Stefanos Togkoulidis <[email protected]>
Date:   Tue Apr 30 20:58:36 2019 +0300

    Just use onFormatChange which now defaults to "force"

commit 6358de3
Merge: a5282ce 5e4d627
Author: Stefanos Togkoulidis <[email protected]>
Date:   Tue Apr 30 20:23:57 2019 +0300

    Merge branch 'master' into rnmobile/fix-list-handling-android

commit 5e4d627
Author: Pascal Birchler <[email protected]>
Date:   Tue Apr 30 18:27:24 2019 +0200

    Update to Babel 7.4 and core-js 3 (#15139)

    * Specify core-js version when using useBuiltIns

    * Update to Babel 7.4 and core-js 3

    * setupFiles: fix path to async-iterator

    * Also update Babel packages in babel-preset-default

    * Update snapshot for babel-preset-default test

    * Manually include web.dom-collections.iterator in jest config

    * Regenerate package-lock.json file after latest changes applied

    * Upgrade ESLint related npm packages

    * Another cleanup in the package-lock.json file

    * Remove ESLint rules which throw error to fix linting

    * Add core-js/modules/web.dom.iterable to make e2e tests work

    * Update Babel packages to the latest version

    * Update ignore files in e2e tests config

    * Add changelog entries related to Babel and ESLint version bumps

commit 1d959ba
Author: Andrés <[email protected]>
Date:   Tue Apr 30 17:14:35 2019 +0200

    Remove not used state (#15224)

commit d715e93
Author: Jon Surrell <[email protected]>
Date:   Tue Apr 30 16:29:09 2019 +0200

    Babel plugin JSX: Implement Fragment handling (#15120)

    Add imports for `<></>` JSX Fragments.

commit 96cad99
Author: Kelly Dwan <[email protected]>
Date:   Tue Apr 30 09:18:54 2019 -0400

    Fix incorrect ID in FocalPointPicker (#15255)

commit c1ba13e
Author: Jon Surrell <[email protected]>
Date:   Tue Apr 30 13:44:50 2019 +0200

    Update Fragment imports in READMEs (#15262)

    Some Fragment imports were incorrect. Fix them.

commit 297c2f4
Author: etoledom <[email protected]>
Date:   Tue Apr 30 13:19:34 2019 +0200

    RNMobile: Fix autoscroll on ListBlock (#15048)

    * RNMobile: Add ability to send extra props (i.e. props needed just for mobile) to the list block.

    * Adding onCaretVerticalPositionChange to RichText via context

    * Passing onCaretVerticalPositionChange via context to RichText for all RichText based blocks

    This makes it not necessary to pass onCaretVerticalPositionChange as a prop directly to RichText from the block component.

    * Update jest snapshot for block-edit

commit 447dd27
Author: Jorge Costa <[email protected]>
Date:   Tue Apr 30 10:47:55 2019 +0100

    Fix copy paste and delete error core paragraph with locking (#14712)

    * Fix: Copy / Paste error core/paragraph with locking

    * Add test case.

    * Update packages/block-library/src/paragraph/edit.js

    Co-Authored-By: jorgefilipecosta <[email protected]>

commit 0b2eb1c
Author: Jon Surrell <[email protected]>
Date:   Tue Apr 30 09:39:29 2019 +0200

    Dependency extraction webpack plugin: Change option name consistent with docs (#15260)

    In the docs, an option name was provided as `requestToHandle`, but the
    actual option in use was `requestToDependency`.

    Update the code and related tests to use `requestToHandle` as described
    in the documentation instead of `requestToDependency`.

commit 506827e
Author: Jon Surrell <[email protected]>
Date:   Tue Apr 30 09:31:38 2019 +0200

    Read script dependencies from generated files (#15124)

    * Rework script registration to use generated deps

    * Deprecate static list of dependencies

    * Rework translation without packages-dependencies.php

    * Include build/*/*.deps.json in plugin zip

    * Add script dependencies undetectable by build tools

    Some scripts have dependencies that are undetectable by the webpack
    plugin used to generate the dependency files. Add these dependencies
    to the generated dependencies.

    * Add `wp-polyfill` dependency via webpack plugin

    Enable `injectPolyfill` option in the webpack plugin instead of manually
    injecting the dependency via PHP on script registration.

    This will enable the polyfill for consumers of wp-scripts default
    webpack config.

    * Improve deprecated message

    * Remove obsolete lib/packages-dependencies.php

commit c2c8276
Author: Daniel Richards <[email protected]>
Date:   Tue Apr 30 13:36:49 2019 +0800

    Use button block appender on group block (#14943)

    * Use button block appender on group block

    * Update group block e2e tests for the new button appender

    * Add additional margin to the appender when a group has a background color.

    * Use more opaque versions of the appender background color

    To ensure greater compatibility when layered on top of a block with a background color.

    * Block Library: Display ButtonBlockAppender only if selected or empty

    * Only render button appender when the group has no inner blocks

commit 1e96ec7
Author: Andrew Duthie <[email protected]>
Date:   Mon Apr 29 16:40:24 2019 -0400

    Build Tooling: Pass individual files as arguments from watch to build script (#15219)

commit cafb57b
Author: Grzegorz (Greg) Ziółkowski <[email protected]>
Date:   Mon Apr 29 22:12:08 2019 +0200

    Blocks: Upgrade simple-html-tokenizer dependency (#15246)

commit 5b3b3ab
Author: Andrew Duthie <[email protected]>
Date:   Mon Apr 29 13:38:09 2019 -0400

    Framework: Add REST API codeowners (#15215)

commit 64c48fd
Author: Ian Dunn <[email protected]>
Date:   Mon Apr 29 10:06:46 2019 -0700

    Correct name of `date_i18n()` PHP function. (#15204)

commit 8c28814
Author: Andrew Duthie <[email protected]>
Date:   Mon Apr 29 13:05:10 2019 -0400

    Framework: Remove TESTS.md (#15217)

commit 0f1fb57
Author: Emmanuel Hesry <[email protected]>
Date:   Mon Apr 29 17:34:22 2019 +0200

    fix typo in withDispatch documentation (#15251)

commit a5282ce
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 16:08:31 2019 +0300

    Revert "Trivial change to trigger Travis"

    This reverts commit e22ffde.

commit e22ffde
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 16:08:24 2019 +0300

    Trivial change to trigger Travis

commit 15a51f8
Author: Jorge Costa <[email protected]>
Date:   Fri Apr 19 15:33:15 2019 +0100

    chore: Fix: Lint error that makes unit tests (and CI tests) fail. (#15073)

commit 2b11074
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 12:42:55 2019 +0300

    Fix lint issues

commit 9c298c8
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 12:01:46 2019 +0300

    Need to specify firedAfterTextChanged on all Aztec events

commit 46e086d
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 03:59:59 2019 +0300

    Force Aztec update if "Enter" fired before text change

commit 724e295
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 03:11:52 2019 +0300

    Differentiate Android and iOS since assumptions diverged

    The iOS side still expects to just check against `this.lastContent` to
    force the change into Aztec.

commit 52386fa
Author: Stefanos Togkoulidis <[email protected]>
Date:   Thu Apr 25 01:55:22 2019 +0300

    Use a flag to signal Aztec-originated changes

    And assume that when that flag is false, component changes need to get
    sent/reflected down to Aztec.

commit c7aa381
Author: Stefanos Togkoulidis <[email protected]>
Date:   Tue Apr 23 17:30:45 2019 +0300

    Able to not lose content

commit 60c75b0
Author: Stefanos Togkoulidis <[email protected]>
Date:   Tue Apr 23 10:09:23 2019 +0300

    If text already changed, don't modify it
@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build Tooling: Watch task can produce error when many files change (ENAMETOOLONG)
4 participants