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

Add some missing package dependencies #41486

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

anomiex
Copy link
Contributor

@anomiex anomiex commented Jun 1, 2022

!-- Thanks for contributing to Gutenberg! Please follow the Gutenberg Contributing Guidelines:
https://github.com/WordPress/gutenberg/blob/trunk/CONTRIBUTING.md -->

What?

Add some missing package dependencies

Why?

Build code transformations can introduce dependencies on packages such
as @wordpress/element and @babel/runtime. These need to be declared if
the package is to function correctly with yarn's p'n'p or pnpm with
hoisting disabled.

How?

Adding the missing dependencies. Normally I'd probably have done the @babel/runtime dep as a peer dep, but I see you use a normal dependency everywhere else so I followed along.

Testing Instructions

  1. Follow the reproduction instructions in the linked bug to set up the test and reproduce the bug.
  2. Do yarn add <package>@file:/path/to/gutenberg/packages/<package> or (with pnpm 7) pnpm add <package>@file:/path/to/gutenberg/packages/<package> to point yarn or pnpm at the locally built version of the package.
  3. Repeat the final instruction in the reproduction to check that the bug is fixed.
@Mamaduka Mamaduka added the [Type] Bug An existing feature does not function as intended label Jun 2, 2022
@skorasaurus skorasaurus added the npm Packages Related to npm packages label Jun 6, 2022
Build code transformations can introduce dependencies on packages such
as @wordpress/element and @babel/runtime. These need to be declared if
the package is to function correctly with yarn's p'n'p or pnpm with
hoisting disabled.

- @wordpress/preferences depends on @wordpress/element (fixes WordPress#41341)
- @wordpress/reusable-blocks depends on @babel/runtime (fixes WordPress#41343)
- @wordpress/viewport depends on @wordpress/element (fixes WordPress#41346)
@anomiex
Copy link
Contributor Author

anomiex commented Jun 20, 2023

@gziolo
Copy link
Member

gziolo commented Aug 7, 2023

Thank you for adding the missing dependencies. Can you think of a programmatic way to prevent this type of issues to happen again?

Normally I'd probably have done the @babel/runtime dep as a peer dep, but I see you use a normal dependency everywhere else so I followed along.

Yes, that should be changed to a peer dependency when we move Node 16+ and the latest npm.

@gziolo gziolo merged commit fdbe9a9 into WordPress:trunk Aug 7, 2023
26 of 48 checks passed
@github-actions github-actions bot added this to the Gutenberg 16.5 milestone Aug 7, 2023
@anomiex anomiex deleted the fix/missing-deps branch August 15, 2023 19:50
@anomiex
Copy link
Contributor Author

anomiex commented Aug 15, 2023

Thank you for adding the missing dependencies. Can you think of a programmatic way to prevent this type of issues to happen again?

Not offhand. You'd probably have to find/make some sort of tooling to extract all the deps from the built files and see that they're either depended on or peer-depended on by the package.

@gziolo
Copy link
Member

gziolo commented Aug 16, 2023

Thank you for adding the missing dependencies. Can you think of a programmatic way to prevent this type of issues to happen again?

Not offhand. You'd probably have to find/make some sort of tooling to extract all the deps from the built files and see that they're either depended on or peer-depended on by the package.

Ok, it makes sense. We have some linting rules in place from eslint-plugin-import that work nicely with the source code, but they don't cover the parts dynamically modified by the Babel plugin that gets published to npm inside build-module folders. We potentially could extract these rules and run them against build-module folder in a separate process on CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Bug An existing feature does not function as intended
4 participants