Declare singleton-ish packages as peer dependencies #26954

jsnajdr · 2020-11-13T09:13:56Z

Implements proposal from #8981 that declares singleton-ish packages as peer dependencies, to make consumers' node_modules trees easier to maintain and less prone to package duplication. Which, in case of singletons inside packages, leads to broken apps.

Any package that contains a singleton is eligible:

data: has the store registry and registerStore method.
i18n: stores translations set with setLocaleData and consumed with __ et al.
hooks: has registry of filters and actions, methods to register and apply them.
rich-text: registers the core/rich-text store that contains registry of format types.
plugins: has registry of plugins and registerPlugin, unregisterPlugin methods.
edit-post: exposes API for plugins like the PluginPrePublishPanel slot that plugins can fill.
block-editor: registers and exposes the core/block-editor store.
blocks: exposes the core/blocks store.
editor: exposes the core/editor store and also statically registers hooks and loops through core/blocks to call shimAttributeSource
notices: exposes the core/notices store.
core-data: exposes the core data store.
reusable-blocks: exposes a core/reusable-blocks that manages the isEditingReusableBlock flags for the reusable block editor.
keyboard-shortcuts: exposes the core/keyboard-shortcuts store where component can register these.
interface: exposes the core/interface store that manages enabled/disabled state for sidebar items.
viewport: exposes the core/viewport store that watches the browser viewport and fires change events to subscribers.

As peer dependencies need to be declared with version numbers instead of file: links that Lerna would automatically replace, I used the latest version in the repo for each declaration.

In future, we'll need to be more careful about maintaining the semver versions of each package whenever a new API is added, maintaining the CHANGELOG.md files (many are neglected and many versions behind), and about tracking which dependent packages are using the new API, updating the peer dependencies and semver versions accordingly.

github-actions · 2020-11-13T09:30:07Z

Size Change: 0 B

Total Size: 1.19 MB

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/annotations/index.js`	3.77 kB	0 B
`build/api-fetch/index.js`	3.42 kB	0 B
`build/autop/index.js`	2.84 kB	0 B
`build/blob/index.js`	664 B	0 B
`build/block-directory/index.js`	8.71 kB	0 B
`build/block-directory/style-rtl.css`	943 B	0 B
`build/block-directory/style.css`	942 B	0 B
`build/block-editor/index.js`	133 kB	0 B
`build/block-editor/style-rtl.css`	11.1 kB	0 B
`build/block-editor/style.css`	11.1 kB	0 B
`build/block-library/editor-rtl.css`	8.91 kB	0 B
`build/block-library/editor.css`	8.91 kB	0 B
`build/block-library/index.js`	147 kB	0 B
`build/block-library/style-rtl.css`	8.1 kB	0 B
`build/block-library/style.css`	8.1 kB	0 B
`build/block-library/theme-rtl.css`	792 B	0 B
`build/block-library/theme.css`	793 B	0 B
`build/block-serialization-default-parser/index.js`	1.87 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.06 kB	0 B
`build/blocks/index.js`	48 kB	0 B
`build/components/index.js`	171 kB	0 B
`build/components/style-rtl.css`	15.3 kB	0 B
`build/components/style.css`	15.3 kB	0 B
`build/compose/index.js`	9.9 kB	0 B
`build/core-data/index.js`	14.8 kB	0 B
`build/data-controls/index.js`	821 B	0 B
`build/data/index.js`	8.74 kB	0 B
`build/date/index.js`	11.2 kB	0 B
`build/deprecated/index.js`	768 B	0 B
`build/dom-ready/index.js`	571 B	0 B
`build/dom/index.js`	4.92 kB	0 B
`build/edit-navigation/index.js`	11.1 kB	0 B
`build/edit-navigation/style-rtl.css`	881 B	0 B
`build/edit-navigation/style.css`	885 B	0 B
`build/edit-post/index.js`	306 kB	0 B
`build/edit-post/style-rtl.css`	6.43 kB	0 B
`build/edit-post/style.css`	6.42 kB	0 B
`build/edit-site/index.js`	23.3 kB	0 B
`build/edit-site/style-rtl.css`	3.95 kB	0 B
`build/edit-site/style.css`	3.95 kB	0 B
`build/edit-widgets/index.js`	26.3 kB	0 B
`build/edit-widgets/style-rtl.css`	3.16 kB	0 B
`build/edit-widgets/style.css`	3.16 kB	0 B
`build/editor/editor-styles-rtl.css`	476 B	0 B
`build/editor/editor-styles.css`	478 B	0 B
`build/editor/index.js`	42.5 kB	0 B
`build/editor/style-rtl.css`	3.85 kB	0 B
`build/editor/style.css`	3.85 kB	0 B
`build/element/index.js`	4.62 kB	0 B
`build/escape-html/index.js`	735 B	0 B
`build/format-library/index.js`	6.86 kB	0 B
`build/format-library/style-rtl.css`	547 B	0 B
`build/format-library/style.css`	548 B	0 B
`build/hooks/index.js`	2.16 kB	0 B
`build/html-entities/index.js`	623 B	0 B
`build/i18n/index.js`	3.57 kB	0 B
`build/is-shallow-equal/index.js`	713 B	0 B
`build/keyboard-shortcuts/index.js`	2.52 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.1 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	476 B	0 B
`build/list-reusable-blocks/style.css`	476 B	0 B
`build/media-utils/index.js`	5.32 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`	671 B	0 B
`build/nux/style.css`	668 B	0 B
`build/plugins/index.js`	2.56 kB	0 B
`build/primitives/index.js`	1.43 kB	0 B
`build/priority-queue/index.js`	790 B	0 B
`build/redux-routine/index.js`	2.83 kB	0 B
`build/reusable-blocks/index.js`	3.05 kB	0 B
`build/rich-text/index.js`	13.3 kB	0 B
`build/server-side-render/index.js`	2.77 kB	0 B
`build/shortcode/index.js`	1.69 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/url/index.js`	4.06 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.22 kB	0 B

_{compressed-size-action}

youknowriad · 2020-11-13T10:06:39Z

packages/a11y/package.json

+		"@wordpress/dom-ready": "file:../dom-ready"
+	},
+	"peerDependencies": {
+		"@wordpress/i18n": "^3.16.0"


Just to make sure I understand properly, when we work locally on the repo, these will use the local versions right and not install things from npm registry?

Because @wordpress/i18n is already declared in the root package.json as a file:package/i18n link, there will be a symlink to packages/i18n in node_modules/@wordpress/i18n. The package from NPM registry will never be installed there, because that would directly contradict what the root package.json says.

NPM 6 (and also Yarn) will never try to install any peer dependency. It only checks if someone else installed it into node_modules already, and will check that the semver matches. And because the package is required by the root package.json, and the version field in the symlinked package's package.json matches, it will be happy.

NPM 7 will also find the dependency satisfied by the symlink in root node_modules.

Only if we removed the i18n dependency from the root package.json (or forgot to add a new package there), that's the only case where unwanted things can happen. NPM 6 will issue a warning that peer dependency was not found. And NPM 7 will install it into root node_modules from the NPM registry.

Is the intention to allow only the same major version of peer dependencies?

Yes, in this case, the a11y package uses the 3.16 version of the i18n API. 3.15 probably lacks some feature that we make use of, so it's not compatible. Version 2 could have completely different API that is not compatible, and 4 might also break everything.

youknowriad

Any thoughts on how this impacts the release package process?

jsnajdr · 2020-11-13T10:16:47Z

While working on this PR and auditing the dependencies, I found some React Native components that should probably be in other packages:

HTMLTextInput in components is connected to the core/editor data store and also registers and unregisters hooks. Such a tightly integrated component shouldn't be in components, but maybe in the editor package instead? The only usage is in edit-post React Native <Layout>. (cc @Tug)
the block-editor package contains React Native components (Inserter, BlockMobileToolbar, PageTemplatePicker) that work with the core/editor data store, introducing a dependency that is backwards (cc @koke)

jsnajdr · 2020-11-13T11:45:43Z

Any thoughts on how this impacts the release package process?

That's something to figure out. I have limited knowledge about how even the existing release process works.

For example, @gziolo recently released a few packages in 3523f7e. How did you know that @wordpress/jest-preset-default needs a minor version bump from 6.4.0 to 6.5.0, while @wordpress/e2e-tests needs only a patch bump from 1.24.1 to 1.24.2? Is that automated, or needs manual audit of the changes?

Peer dependencies should not be bumped automatically whenever a new version of the package is released. Bugfix/patch releases never need a peer bump. If a package declares dependency on ^1.2.3 and 1.2.4 is released, the consumer's package manager will automaticaly use the new version.

And if there is a minor release with new features, consumers should bump the peer dependency only if they really use the new API and can't work with the older version. Bumping the peer dependency means a major version bump for the consumer. Just like many NPM packages release a major version just because they removed support for old versions of Node.js.

This "package new API -> consumer major release" transition could have a ripple effect through several packages, triggering many major updates in one release cycle.

gziolo · 2020-11-13T13:35:17Z

Thank you @jsnajdr for opening this proposal. It's a lot to process in terms of publishing packages. I'll do my best to explain but it might require some follow-ups 😅

For example, @gziolo recently released a few packages in 3523f7e. How did you know that @wordpress/jest-preset-default needs a minor version bump from 6.4.0 to 6.5.0, while @wordpress/e2e-tests needs only a patch bump from 1.24.1 to 1.24.2? Is that automated, or needs manual audit of the changes?

In this particular case, I might selected version bumps manually as it was a bug fix release 😄

Usually, it's automated. There is a very naive implementation that picks version bumps for modified packages based on entries in CHANGELOG.md files:

gutenberg/bin/plugin/commands/packages.js

Lines 113 to 155 in 9db42e0

    
           for await ( const line of lines ) { 
        
           	const lineNormalized = line.toLowerCase(); 
        
           	// Detect unpublished changes first. 
        
           	if ( lineNormalized.startsWith( '## unreleased' ) ) { 
        
           		changesDetected = true; 
        
           		continue; 
        
           	} 
        
           	// Skip all lines until unpublished changes found. 
        
           	if ( ! changesDetected ) { 
        
           		continue; 
        
           	} 
        
           	// A previous published version detected. Stop processing. 
        
           	if ( lineNormalized.startsWith( '## ' ) ) { 
        
           		break; 
        
           	} 
        
           	// A major version bump required. Stop processing. 
        
           	if ( lineNormalized.startsWith( '### breaking change' ) ) { 
        
           		versionBump = 'major'; 
        
           		break; 
        
           	} 
        
           	// A minor version bump required. Proceed to the next line. 
        
           	if ( 
        
           		lineNormalized.startsWith( '### deprecation' ) || 
        
           		lineNormalized.startsWith( '### enhancement' ) || 
        
           		lineNormalized.startsWith( '### new feature' ) 
        
           	) { 
        
           		versionBump = 'minor'; 
        
           		continue; 
        
           	} 
        
           	// A version bump required. Found new changelog section. 
        
           	if ( 
        
           		versionBump !== 'minor' && 
        
           		( lineNormalized.startsWith( '### ' ) || 
        
           			lineNormalized.startsWith( '- initial release' ) ) 
        
           	) { 
        
           		versionBump = minimumVersionBump; 
        
           	} 
        
           }

The same script also updates package.json files for the same packages so Lerna could pick them up. The trick here is that appending -prerelease string ensures that Lerna doesn't bump the version again (it removes that part no matter what type of version bump is selected for all packages, usually minor or fix). Code:

gutenberg/bin/plugin/commands/packages.js

Lines 218 to 226 in 9db42e0

    
           const packageJson = readJSONFile( packageJSONPath ); 
        
           const newPackageJson = { 
        
           	...packageJson, 
        
           	version: nextVersion + '-prerelease', 
        
           }; 
        
           fs.writeFileSync( 
        
           	packageJSONPath, 
        
           	JSON.stringify( newPackageJson, null, '\t' ) + '\n' 
        
           );

In general, it seems like it could be extended to update also peerDependencies with new versions when really necessary. The only issue is that it doesn't cover packages where CHANGELOG.md remains unchanged and this is a very common scenario for packages specific to the editor.

And if there is a minor release with new features, consumers should bump the peer dependency only if they really use the new API and can't work with the older version.

It might be really difficult to verify if a new API is used in the package. It would be also problematic when during the release minor or major version bump would be selected for all packages. We enforce minor version bump after every major WordPress release to leave room for security fixes for older versions of WordPress. So technically, there might not be a valid reason to change versions of peer dependencies. It's all complex but I'm sure is still doable.

I think it's essential to have a way to automate peer dependencies updates because otherwise, they won't be very useful when consuming packages from npm.

Tug · 2020-11-13T18:26:44Z

HTMLTextInput in components is connected to the core/editor data store and also registers and unregisters hooks. Such a tightly integrated component shouldn't be in components, but maybe in the editor package instead? The only usage is in edit-post React Native <Layout>.

Indeed, I addressed this in a PR which targets this branch: #26969
I still need to make sure we're not breaking e2e tests (or other things) with that change.

the block-editor package contains React Native components (Inserter, BlockMobileToolbar, PageTemplatePicker) that work with the core/editor data store, introducing a dependency that is backwards

PageTemplatePicker is indeed misplaced. I can work on a fix for this as well 👍

About Inserter and BlockMobileToolbar, they're at the right place imo, though they use a selector from the core/editor store that's not available in block-editor: namely getClipboard(). The latter should probably be updated, or we could have some store to store communication (forgot how to do this right now but I'll have a deeper look next week)

Tug · 2020-11-16T19:32:36Z

Had a look at clipboard and was surprised this is actually a native only reducer. It seems like we missed the opportunity to implement our own version of CopyHandler and ClipboardButton when we implemented copy/paste.

geriux · 2021-01-04T09:32:12Z

Hey there 👋

the block-editor package contains React Native components (Inserter, BlockMobileToolbar, PageTemplatePicker) that work with the core/editor data store, introducing a dependency that is backwards (cc @koke)

For the components Inserter and BlockMobileToolbar the usage of core/editor data store has been removed in this PR. As for the PageTemplatePicker, it will be removed altogether in this PR.

paaljoachim · 2021-02-20T22:41:46Z

Hello everyone. What is the next step for this PR?
How are things going @jsnajdr Jarda?

jsnajdr · 2021-02-22T10:37:16Z

@paaljoachim I think the next step is here is to upgrade Gutenberg to NPM 7 which has a better algorithm for installing peer dependencies. This comment contains a detailed analysis of how NPM 6 and 7 and Yarn work with peer deps: #8981 (comment)

paaljoachim · 2021-02-22T11:40:13Z

Thank you @jsnajdr Jarda.

I will ping @ockham and @noahtallen and let them know about this PR.

paaljoachim · 2021-02-22T12:18:48Z

Hi @jsnajdr Jarda
I happen to come across this: #29204

jsnajdr · 2021-02-22T13:19:36Z

I happen to come across this: #29204

My understanding is that PR prevents a wild uncontrolled upgrade to npm 7 rather than intending to stay on 6 for a prolonged period of time.

If you wanted to point out that npm should be spelled in all lowercase -- thank you, I didn't know that and was always little bit confused when typing the three letters 🙂 Now I know that npm is just like webpack -- no capital letters.

gziolo · 2021-02-22T13:40:48Z

If you wanted to point out that npm should be spelled in all lowercase -- thank you, I didn't know that and was always little bit confused when typing the three letters 🙂 Now I know that npm is just like webpack -- no capital letters.

@jsnajdr that's a good reflection, but I think the PR is trying to downgrade npm version to 6 to avoid failures with installing dependencies for Gutenberg when using npm 7 😄

jsnajdr · 2021-02-22T13:46:20Z

@gziolo The @paaljoachim's comment originally linked to this specific comment: #29204 (comment)

That's why I was not sure if it's about npm 6 or about spelling 😄

youknowriad · 2024-04-25T21:57:38Z

Is this still something we want to do? Probably better to close the PR and reopen if needed anyway?

github-actions · 2024-04-25T21:57:50Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @Tug.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: Tug.

Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: geriux <[email protected]>
Co-authored-by: paaljoachim <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

jsnajdr · 2024-04-26T08:14:55Z

I think this is still a desirable thing to do, but the actual diff is hopelessly outdated. It will be easier to create it from scratch than trying to rebase the existing one.

Back in 2020 we were still using old Node 14 and NPM 6, nowadays peerDependencies behave quite differently, and better.

jsnajdr added the npm Packages Related to npm packages label Nov 13, 2020

jsnajdr requested review from adamziel, youknowriad, gziolo and jorgefilipecosta November 13, 2020 09:13

jsnajdr requested review from adamsilverstein, ajitbohra, atimmer, cameronvoell, chrisvanpatten, draganescu, ellatrix, etoledom, guarani, kevin940726, mmtr, nerrad, noisysocks, ntwb, Soean, talldan and tellthemachines as code owners November 13, 2020 09:13

jsnajdr self-assigned this Nov 13, 2020

jsnajdr added 3 commits November 13, 2020 10:20

Reformat packages/react-native-editor/package.json with tabs

f6daad4

Remove unneeded babel/runtime dependency from Babel plugins and presets

273b64e

Declare singleton-ish packages as peer dependencies

9340e51

jsnajdr force-pushed the convert/peer-dependencies branch from 89e6408 to 9340e51 Compare November 13, 2020 09:21

youknowriad reviewed Nov 13, 2020

View reviewed changes

jsnajdr mentioned this pull request Nov 18, 2020

Data: Use store instance as param for select and dispatch #26655

Merged

6 tasks

geriux mentioned this pull request Nov 25, 2020

[Mobile] Copy/Paste improvement #27242

Merged

10 tasks

jsnajdr mentioned this pull request Dec 4, 2020

The Slot/Fill feature depends on a singleton registry in components package #27191

Open

etoledom removed request for ellatrix, kevin940726, tellthemachines, etoledom and jorgefilipecosta December 8, 2020 16:40

Base automatically changed from master to trunk March 1, 2021 15:44

jsnajdr requested a review from getdave as a code owner May 17, 2021 16:09

guarani removed their request for review January 19, 2022 13:02

jsnajdr closed this Apr 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declare singleton-ish packages as peer dependencies #26954

Declare singleton-ish packages as peer dependencies #26954

jsnajdr commented Nov 13, 2020

github-actions bot commented Nov 13, 2020

youknowriad Nov 13, 2020

jsnajdr Nov 13, 2020

gziolo Nov 13, 2020

jsnajdr Nov 13, 2020

youknowriad left a comment

jsnajdr commented Nov 13, 2020

jsnajdr commented Nov 13, 2020

gziolo commented Nov 13, 2020

Tug commented Nov 13, 2020 •

edited

Loading

Tug commented Nov 16, 2020

geriux commented Jan 4, 2021

paaljoachim commented Feb 20, 2021

jsnajdr commented Feb 22, 2021

paaljoachim commented Feb 22, 2021

paaljoachim commented Feb 22, 2021 •

edited

Loading

jsnajdr commented Feb 22, 2021

gziolo commented Feb 22, 2021

jsnajdr commented Feb 22, 2021

youknowriad commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

jsnajdr commented Apr 26, 2024

Declare singleton-ish packages as peer dependencies #26954

Declare singleton-ish packages as peer dependencies #26954

Conversation

jsnajdr commented Nov 13, 2020

github-actions bot commented Nov 13, 2020

youknowriad Nov 13, 2020

Choose a reason for hiding this comment

jsnajdr Nov 13, 2020

Choose a reason for hiding this comment

gziolo Nov 13, 2020

Choose a reason for hiding this comment

jsnajdr Nov 13, 2020

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

jsnajdr commented Nov 13, 2020

jsnajdr commented Nov 13, 2020

gziolo commented Nov 13, 2020

Tug commented Nov 13, 2020 • edited Loading

Tug commented Nov 16, 2020

geriux commented Jan 4, 2021

paaljoachim commented Feb 20, 2021

jsnajdr commented Feb 22, 2021

paaljoachim commented Feb 22, 2021

paaljoachim commented Feb 22, 2021 • edited Loading

jsnajdr commented Feb 22, 2021

gziolo commented Feb 22, 2021

jsnajdr commented Feb 22, 2021

youknowriad commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

Unlinked Accounts

jsnajdr commented Apr 26, 2024

Tug commented Nov 13, 2020 •

edited

Loading

paaljoachim commented Feb 22, 2021 •

edited

Loading