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

Prettier Config: Add type-checking #21053

Merged
merged 2 commits into from
Apr 1, 2020
Merged

Prettier Config: Add type-checking #21053

merged 2 commits into from
Apr 1, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 20, 2020

Blocked by (current base): #18942
Related: #18838

This pull request seeks to add type-checking for the @wordpress/prettier-config package.

By virtue of #18942, it also implies that types will be output as part of package distribution, though this may or may not be a very useful package to reference.

The primary benefit of these changes is largely in verifying configuration keys and values.

It also benefits from the same sorts of thing I wrote about recently, wherein the addition of the type detail trivializes future configuration maintenance:

Prettier config

Finally, this pull request serves as a possible reference for the revised approach to opting in to TypeScript type-checking for a package as of #18942.

Testing Instructions:

Ensure type-checking passes:

npm run build:package-types

@aduth aduth added [Type] Enhancement A suggestion for improvement. [Package] Prettier config /packages/prettier-config labels Mar 20, 2020
@aduth aduth requested review from gziolo and ntwb March 20, 2020 18:48
@aduth aduth added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Mar 20, 2020
@github-actions
Copy link

github-actions bot commented Mar 20, 2020

Size Change: +13.5 kB (1%)

Total Size: 883 kB

Filename Size Change
build/a11y/index.js 1.02 kB +19 B (1%)
build/annotations/index.js 3.45 kB +1 B
build/api-fetch/index.js 3.8 kB +410 B (10%) ⚠️
build/block-directory/index.js 6.02 kB +2 B (0%)
build/block-editor/index.js 102 kB +12 B (0%)
build/block-editor/style-rtl.css 11.2 kB +171 B (1%)
build/block-editor/style.css 11.2 kB +175 B (1%)
build/block-library/index.js 110 kB -148 B (0%)
build/blocks/index.js 57.5 kB -1 B
build/components/style-rtl.css 16.1 kB +296 B (1%)
build/components/style.css 16 kB +297 B (1%)
build/core-data/index.js 10.7 kB +1 B
build/data-controls/index.js 1.03 kB -1 B
build/edit-post/index.js 92.3 kB -1 B
build/edit-post/style-rtl.css 12 kB +3.67 kB (30%) 🚨
build/edit-post/style.css 12 kB +3.67 kB (30%) 🚨
build/edit-site/index.js 9.04 kB +11 B (0%)
build/edit-site/style-rtl.css 4.62 kB +1.2 kB (26%) 🚨
build/edit-site/style.css 4.61 kB +1.2 kB (26%) 🚨
build/edit-widgets/style-rtl.css 3.74 kB +1.17 kB (31%) 🚨
build/edit-widgets/style.css 3.74 kB +1.17 kB (31%) 🚨
build/editor/index.js 42.8 kB -1 B
build/editor/style-rtl.css 3.47 kB +84 B (2%)
build/editor/style.css 3.47 kB +81 B (2%)
build/keyboard-shortcuts/index.js 2.3 kB +1 B
build/media-utils/index.js 4.84 kB -2 B (0%)
build/server-side-render/index.js 2.54 kB -3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/autop/index.js 2.59 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/editor-rtl.css 7.21 kB 0 B
build/block-library/editor.css 7.21 kB 0 B
build/block-library/style-rtl.css 7.5 kB 0 B
build/block-library/style.css 7.51 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/index.js 195 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/data/index.js 8.23 kB 0 B
build/date/index.js 5.37 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.05 kB 0 B
build/edit-navigation/index.js 2.4 kB 0 B
build/edit-navigation/style-rtl.css 95 B 0 B
build/edit-navigation/style.css 95 B 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 426 B 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.94 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 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keycodes/index.js 1.7 kB 0 B
build/list-reusable-blocks/index.js 2.99 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/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 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.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 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.01 kB 0 B
build/viewport/index.js 1.6 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

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.

I assume the config file included is fine.

Noting that, It’s yet another thing to take into account when creating a package. Should we introduce package scaffolding tool? 😅

packages/prettier-config/lib/index.js Show resolved Hide resolved
packages/prettier-config/lib/index.js Show resolved Hide resolved
@aduth
Copy link
Member Author

aduth commented Mar 23, 2020

Noting that, It’s yet another thing to take into account when creating a package. Should we introduce package scaffolding tool? 😅

Yeah, part of this was to just to demonstrate (even for myself) the amount of effort which would be involved as of the changes resulting from #18942. I think a scaffolding tool could be useful. I was also considering whether it could be the sort of thing where a bot would comment on a pull request where a new package is being added, which could do a few things:

  • Include a checklist of tasks to complete (and automatically update them as checked once completed)
  • Reference relevant documentation
  • Ping the core development team (since it affects a public interface)

In theory, a bot could even do the commits itself.

@aduth
Copy link
Member Author

aduth commented Mar 24, 2020

Just to reiterate: This is currently blocked by #18942. My plan would be to wait for #18942 to be merged, then change the base of this pull request to master and merge.

@gziolo
Copy link
Member

gziolo commented Mar 24, 2020

It sounds like a plan 👍

I want to use this PR to add types for create block a scripts packages at some point in the near future 😃

@aduth
Copy link
Member Author

aduth commented Mar 25, 2020

@sirreal I see you did a force push recently that introduced quite a few unexpected changes and merge conflicts. Do you know what might have happened?

@sirreal
Copy link
Member

sirreal commented Mar 25, 2020

I force pushed to the base branch (#18942) after rebasing against master to fix conflicts.

@aduth
Copy link
Member Author

aduth commented Mar 25, 2020

Ah, I guess I didn't anticipate those sorts of changes in your branch would surface up so prominently in this pull request 🤔

No matter, I should be able to get it resolved without much trouble. Likely just needs a rebase against your newly-rebased branch.

@sirreal
Copy link
Member

sirreal commented Mar 25, 2020

I've pushed more changes to the base branch and appear to have introduced more conflicts. Sorry!

It's a sign that it's time to land #18942 😁

@aduth
Copy link
Member Author

aduth commented Mar 26, 2020

It's a sign that it's time to land #18942 😁

Yes 😄 , but also....

I've pushed more changes to the base branch and appear to have introduced more conflicts. Sorry!

It looks bad, but I doubt there are actually any conflicts to resolve.

@aduth aduth changed the base branch from try/output-typedefs to master March 31, 2020 19:35
@aduth
Copy link
Member Author

aduth commented Mar 31, 2020

Noting that I'm intentionally choosing not to include a CHANGELOG entry for this change because there isn't yet a published version of this package (i.e. it technically still qualified under the current "initial release" note).

@aduth aduth force-pushed the add/types-prettier-config branch from 8dee3d0 to 22803cb Compare March 31, 2020 20:45
@aduth
Copy link
Member Author

aduth commented Apr 3, 2020

I neglected to include necessary changes here. Don't use this pull request as reference 😬

See #21381

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Prettier config /packages/prettier-config [Status] Blocked Used to indicate that a current effort isn't able to move forward [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants