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.

Sign up for GitHub

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

Jump to bottom

Build Tooling: Run packages build before lint #22088

Merged
merged 1 commit into from
May 5, 2020
Merged

Build Tooling: Run packages build before lint #22088

merged 1 commit into from
May 5, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented May 4, 2020 •
edited by oandregal
Loading

Related: #21864 (comment)
Related (link requires registration): https://wordpress.slack.com/archives/C5UNMSU4R/p1588614936461800

This pull request seeks to resolve an issue where the eslint-plugin-import/no-extraneous-dependencies rule will fail to execute correctly in Travis, leading to false negatives where a missing dependency may be introduced (see #21864 (comment), #22086).

Implementation Notes:

From some further debugging, it appears that eslint-plugin-import may depend on package.json's main field to be resolveable:

Prior to these changes, there was no build being run. Each package typically defines it entrypoint as "main": "build/index.js", which does not exist prior to a build in a clean installation.

For future consideration: Ideally we don't need to run the build as part of the lint installation step, since it does add some time to the job's runtime. If we can find some other way to satisfy the ESLint plugin's resolver behavior which doesn't require resolving the built file, it would help to reduce the overall Travis build time.

Testing Instructions:

I have temporarily included a revert of the commit ab4166f in order to test these changes. Edit: Removed after verification at #22088 (comment).

Testing consists of:

  1. ~Verifying that Travis fails as expected, while the commit is left to remain in the branch history.~~ Edit: Verified at Build Tooling: Run packages build before lint #22088 (comment).
  2. Verifying locally that lint passes (unexpectedly) in a clean installation, but fails once the minimal build is run:
git revert ab4166f
npm run clean:packages
npm run lint-js packages/block-editor/src/components/rich-text/index.js
# No errors
node ./bin/packages/build.js
npm run lint-js packages/block-editor/src/components/rich-text/index.js
# error  '@wordpress/shortcode' should be listed in the project's dependencies.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling labels May 4, 2020
@aduth aduth requested a review from oandregal May 4, 2020 18:41
@aduth aduth mentioned this pull request May 4, 2020
Merged
@github-actions
Copy link

github-actions bot commented May 4, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 821 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.6 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 101 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.08 kB 0 B
build/block-library/editor.css 7.08 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.24 kB 0 B
build/block-library/style.css 7.25 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 179 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 4.05 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.3 kB 0 B
build/edit-site/style-rtl.css 5.19 kB 0 B
build/edit-site/style.css 5.2 kB 0 B
build/edit-widgets/index.js 8.33 kB 0 B
build/edit-widgets/style-rtl.css 4.67 kB 0 B
build/edit-widgets/style.css 4.66 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@aduth
Copy link
Member Author

aduth commented May 4, 2020

Confirmed expected failure in Travis:

[0] /home/travis/build/WordPress/gutenberg/packages/block-editor/src/components/rich-text/index.js
[0] 42:1 error '@wordpress/shortcode' should be listed in the project's dependencies. Run >'npm i -S @wordpress/shortcode' to add it import/no-extraneous-dependencies

https://travis-ci.com/github/WordPress/gutenberg/jobs/327580972

I'll rebase the branch to drop the commit, but for local testing, it can be "restored" by reverting ab4166f again in the local branch:

git revert ab4166f

@aduth aduth force-pushed the fix/travis-eslint-import-extraneous branch from 6989e3d to e931307 Compare May 4, 2020 21:48
@oandregal
Copy link
Member

oandregal commented May 5, 2020 •
edited
Loading

In thinking through this, I believe I know why it didn't fail for me locally at the time: from a few months ago I've been having issues with the pre-commit hook and it's not running for me.

I guess this is a perfect storm kind of issue: this could only go unrealized to someone that wasn't able to run the pre-commit hook. Now, instead of being ashamed I feel privileged to being able to help uncover the issue :D

@aduth
Copy link
Member Author

aduth commented May 5, 2020

In thinking through this, I believe I know why it didn't fail for me locally at the time: from a few months ago I've been having issues with the pre-commit hook and it's not running for me.

Are you (were you) able to upgrade your copy of Git?

@aduth aduth merged commit ff98feb into master May 5, 2020
@aduth aduth deleted the fix/travis-eslint-import-extraneous branch May 5, 2020 18:13
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone May 5, 2020
@oandregal
Copy link
Member

Are you (were you) able to upgrade your copy of Git?

I've just realized that there is an official repository that maintains the newer versions of git and works in my OS. For some reason, when I last checked, the only option I found was compiling git myself, which I wasn't very fond of. I wish I knew this months ago! I just updated and got my pre-commit hooks back 💪

@gziolo
Copy link
Member

gziolo commented May 12, 2020

This interesting, thank you for exploring it. It feels like we need a custom resolver for WordPress packages to work on the source rather than using the build files. It would be very similar approach to how unit tests are set up.

This PR also explains why I had failures only on Travis in #20905 that tried to add a new rule from the same plugin.

@gziolo gziolo mentioned this pull request May 12, 2020
Merged
6 tasks
@ceyhun ceyhun mentioned this pull request Jan 7, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oandregal oandregal oandregal approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees
No one assigned
Labels
[Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Milestone
Gutenberg 8.1
Development

Successfully merging this pull request may close these issues.
3 participants
@aduth @oandregal @gziolo