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

Packages: Extract @wordpress/scripts-utils package #21503

Closed
wants to merge 1 commit into from
Closed

Packages: Extract @wordpress/scripts-utils package #21503

wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 9, 2020 •
edited
Loading

Description

New npm package @wordpress/scripts-utils that exposed general-purpose utilities to use with other packages. Methods included:

  • fromProjectRoot
  • getArgFromCLI
  • getArgsFromCLI
  • getFileArgsFromCLI
  • hasArgInCLI
  • hasFileArgInCLI
  • hasPackageProp
  • hasProjectFile

I plan to document all those methods separately together with TypeScript in JSDoc integration.

How has this been tested?

Travis should be green and all existing wp-scripts should work as before.

In fact, we use almost all features included in wp-scripts so it self-tests :)

Checklist:

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

@gziolo gziolo self-assigned this Apr 9, 2020
@gziolo gziolo added npm Packages Related to npm packages [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible labels Apr 9, 2020
@gziolo gziolo force-pushed the try/extract-wordpress-scripts-utils branch from be8c816 to c804aa1 Compare April 9, 2020 12:26
@github-actions
Copy link

github-actions bot commented Apr 9, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 845 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.24 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 105 kB 0 B
build/block-editor/style-rtl.css 10 kB 0 B
build/block-editor/style.css 10 kB 0 B
build/block-library/editor-rtl.css 7.05 kB 0 B
build/block-library/editor.css 7.05 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.14 kB 0 B
build/block-library/style.css 7.14 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 57.7 kB 0 B
build/components/index.js 198 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.43 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 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 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.2 kB 0 B
build/edit-post/style-rtl.css 12.4 kB 0 B
build/edit-post/style.css 12.4 kB 0 B
build/edit-site/index.js 10.8 kB 0 B
build/edit-site/style-rtl.css 5.24 kB 0 B
build/edit-site/style.css 5.24 kB 0 B
build/edit-widgets/index.js 8.33 kB 0 B
build/edit-widgets/style-rtl.css 5 kB 0 B
build/edit-widgets/style.css 5 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 43.4 kB 0 B
build/editor/style-rtl.css 3.27 kB 0 B
build/editor/style.css 3.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 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 711 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.12 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.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 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.17 kB 0 B

compressed-size-action

@gziolo gziolo force-pushed the try/extract-wordpress-scripts-utils branch from c804aa1 to 66ac342 Compare April 9, 2020 12:56
@gziolo gziolo mentioned this pull request Apr 9, 2020
Closed
@gziolo gziolo marked this pull request as ready for review April 9, 2020 13:54
@gziolo gziolo requested review from aduth and sirreal April 9, 2020 14:44
@aduth
Copy link
Member

aduth commented Apr 13, 2020 •
edited
Loading

I'm typically not opposed to breaking down shared code to small shared libraries. I do feel the use-cases are pretty limited and solving very specific problems. There's probably not much usability outside these modules? I could see it being more useful if the methods (and the package itself) were targeted toward more generic "does X file exist in the project directory", "is X given as a CLI argument". Even these are pretty unique / distinct. Some of them exist here, but then there are others which are very specific like hasPrettierConfig.

Also, I've been trying unsuccessfully to kill off camelCaseDash for years, and it only seems to propagate more over time 😄 (related: #14869 (comment), https://github.com/WordPress/gutenberg/pull/13814/files#r258035009)

@gziolo
Copy link
Member Author

gziolo commented Apr 13, 2020

There's probably not much usability outside these modules?

Well, I can’t counter argument this 😃

Some of them exist here, but then there are others which are very specific like hasPrettierConfig.

The one mentioned is the most important one.

Also, I've been trying unsuccessfully to kill off camelCaseDash for years, and it only seems to propagate more over time 😄 (related: #14869 (comment), https://github.com/WordPress/gutenberg/pull/13814/files#r258035009)

As noted, it plays its role in a few places for better or worse 😅

@gziolo gziolo mentioned this pull request Apr 15, 2020
Merged
4 tasks
@gziolo
Copy link
Member Author

gziolo commented Apr 15, 2020

@aduth, I opened #21602 to address the part for Prettier. It turned out to be a very elegant solution ✨

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Apr 15, 2020
@gziolo gziolo force-pushed the try/extract-wordpress-scripts-utils branch 6 times, most recently from da2a9f0 to ba42a81 Compare April 20, 2020 08:41
@gziolo gziolo added [Tool] WP Scripts /packages/scripts and removed [Status] In Progress Tracking issues with work in progress labels Apr 20, 2020
@gziolo gziolo mentioned this pull request Apr 20, 2020
Merged
@gziolo gziolo force-pushed the try/extract-wordpress-scripts-utils branch from ba42a81 to c4da841 Compare April 24, 2020 10:58
@aduth
Copy link
Member

aduth commented Apr 24, 2020 •
edited
Loading

Something still isn't sitting quite well with me about this. I think it's my dislike of the term "utils", which to me almost always may as well be substituted with "stuff" as assorted miscellany. And I don't think assorted miscellany makes for a good package. For me, I'd prefer if we'd do one of:

  • Either figure out what this "stuff" is meant to represent, and name it as such.
  • Or keep it internal to @wordpress/scripts, if it really is just script's stuff.

As an extension of previous conversations, I think I can see how it could be organized in a way where we could have one (or even more) packages for:

  • CLI argument interpretation
  • Extracting values from package.json
  • Checking for presence of files in a particular directory
    • And/or resolving from a "root" folder

@gziolo
Copy link
Member Author

gziolo commented Apr 26, 2020

I think it's my dislike of the term "utils", which to me almost always may as well be substituted with "stuff" as assorted miscellany. And I don't think assorted miscellany makes for a good package.

I have to agree with you. In the past, I disbanded wp.utils (before packages) and I split it into small packages mostly because it was a random group of utils that didn’t fit anywhere else... I have sometimes a similar thoughts about @wordpress/components that became a package to go when contributors want to share code. I’m seeing now another package that starts to play a similar role: @wordpress/interface

I like your proposal of how those utilities could be divided. I guess it’s find to wait until we really want to reuse this functionality in Gutenberg.

Thank you for all the feedback, let’s close this PR. I merged 2 PRs regardless with your help that solve some of the issues that triggered this exploration 😃

@gziolo gziolo closed this Apr 26, 2020
@gziolo gziolo deleted the try/extract-wordpress-scripts-utils branch April 26, 2020 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@youknowriad youknowriad Awaiting requested review from youknowriad

@sirreal sirreal Awaiting requested review from sirreal

Assignees

@gziolo gziolo

Labels
npm Packages Related to npm packages [Tool] WP Scripts /packages/scripts [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Milestone
No milestone
Development

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