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: Fix all missing or obsolete dependencies in packages #16969

Merged
merged 4 commits into from Aug 9, 2019
Merged

Packages: Fix all missing or obsolete dependencies in packages #16969

merged 4 commits into from Aug 9, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 8, 2019 •
edited

Description

I discovered that we have some differences in the list of dependencies for WordPress packages. In particular, I have some issues integrating all recently published npm packages with WordPress core. Related ticket:
https://core.trac.wordpress.org/ticket/47843

How has this been tested?

In general, on Gutenberg side, it all should be transparent as we have build tools which properly generate dependencies consumed by WordPress. In core, we have to do it manually so that might be why it doesn't quite work as necessary.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention npm Packages Related to npm packages labels Aug 8, 2019
@gziolo gziolo requested review from youknowriad and a team August 8, 2019 15:25
@gziolo gziolo self-assigned this Aug 8, 2019
@gziolo gziolo force-pushed the update/packages-dependencies branch from 1e34cdb to 000de40 Compare August 8, 2019 15:29
@gziolo gziolo force-pushed the update/packages-dependencies branch from 000de40 to bf67453 Compare August 8, 2019 15:32
gziolo
gziolo commented Aug 8, 2019
View reviewed changes
.eslintrc.js
'import/no-extraneous-dependencies': 'error',
},
excludedFiles: [
'**/*.@(android|ios|native).js',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hypest and @koke - I disabled native files for now to fix all the issues which web version has, but we definitely should revisit it really soon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a follow-up: #16978

@ellatrix
Copy link
Member

ellatrix commented Aug 8, 2019

I wonder if there's any way we can check the JS files for imports and error if the dependency is not found in the package.json file, and also error if there are dependencies that are listed and not used.

@gziolo
Copy link
Member Author

gziolo commented Aug 8, 2019

I wonder if there's any way we can check the JS files for imports and error if the dependency is not found in the package.json file, and also error if there are dependencies that are listed and not used.

So this new ESLint rule (import/no-extraneous-dependencies) I added in this PR solves the first case :)
I couldn't find anything which covers 2nd scenario.

@ellatrix
Copy link
Member

ellatrix commented Aug 9, 2019

I couldn't find anything which covers 2nd scenario.

How did you know which dependencies to remove then?

ellatrix
ellatrix approved these changes Aug 9, 2019
View reviewed changes
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@gziolo
Copy link
Member Author

gziolo commented Aug 9, 2019

I couldn't find anything which covers 2nd scenario.

How did you know which dependencies to remove then?

It was a manual process. I went through build folder where we have JSON files generated with dependencies for each module which are used in the code. Based on that, I removed those WP packages which weren't listed. It would be great to have this automated with a linter rule.

@gziolo gziolo merged commit a12ac00 into master Aug 9, 2019
@gziolo gziolo deleted the update/packages-dependencies branch August 9, 2019 08:48
@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 9, 2019
@gziolo gziolo mentioned this pull request Aug 9, 2019
Closed
@jorgefilipecosta jorgefilipecosta mentioned this pull request Aug 9, 2019
Closed
5 tasks
This was referenced Aug 23, 2019
Closed
Closed
gziolo added a commit that referenced this pull request Aug 29, 2019
@gziolo
Packages: Fix all missing or obsolete dependencies in packages (#16969)
a518717 
* Packages: Fix all missing or obsolete dependencies in packages

* Enable import/no-extraneous-dependencies rule and fix reported violations

* Remove 2 dependencies not used in the project on the global level

* Remove unused @wordpress/core-data dependnecy from @wordpress/block-editor
gziolo added a commit that referenced this pull request Aug 29, 2019
@gziolo
Packages: Fix all missing or obsolete dependencies in packages (#16969)
2fe7dcf 
* Packages: Fix all missing or obsolete dependencies in packages

* Enable import/no-extraneous-dependencies rule and fix reported violations

* Remove 2 dependencies not used in the project on the global level

* Remove unused @wordpress/core-data dependnecy from @wordpress/block-editor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad left review comments

@ellatrix ellatrix ellatrix approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@daniloercoli daniloercoli Awaiting requested review from daniloercoli

@etoledom etoledom Awaiting requested review from etoledom

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@nerrad nerrad Awaiting requested review from nerrad

@SergioEstevao SergioEstevao Awaiting requested review from SergioEstevao

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

@dmsnell dmsnell Awaiting requested review from dmsnell

@oandregal oandregal Awaiting requested review from oandregal

@ntwb ntwb Awaiting requested review from ntwb

Assignees

@gziolo gziolo

Labels
npm Packages Related to npm packages [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 6.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gziolo @ellatrix @youknowriad