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

Native: Enabled import validation for native files #16978

Closed
wants to merge 2 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 9, 2019

Description

Follow-up for #16969.

This PR enables ESLint validation for native code which detects missing dependencies declaration for all packages.

Testing

npm run lint-js

It currently reports 6 errors at the moment which uncover some code issues where packages probably shouldn't be used. See the list of all errors:

gziolo@Grzegorzs-MacBook-Pro gutenberg % npm run lint-js

> gutenberg@6.7.0 lint-js /Users/gziolo/PhpstormProjects/gutenberg
> wp-scripts lint-js


/Users/gziolo/PhpstormProjects/gutenberg/packages/components/src/mobile/html-text-input/index.native.js
  11:1  error  '@wordpress/blocks' should be listed in the project's dependencies. Run 'npm i -S @wordpress/blocks' to add it  import/no-extraneous-dependencies
  12:1  error  '@wordpress/data' should be listed in the project's dependencies. Run 'npm i -S @wordpress/data' to add it      import/no-extraneous-dependencies

/Users/gziolo/PhpstormProjects/gutenberg/packages/edit-post/src/index.native.js
  11:1  error  '@wordpress/format-library' should be listed in the project's dependencies. Run 'npm i -S @wordpress/format-library' to add it  import/no-extraneous-dependencies

/Users/gziolo/PhpstormProjects/gutenberg/packages/rich-text/src/component/index.native.js
  19:1  error  '@wordpress/blocks' should be listed in the project's dependencies. Run 'npm i -S @wordpress/blocks' to add it                import/no-extraneous-dependencies
  20:1  error  '@wordpress/html-entities' should be listed in the project's dependencies. Run 'npm i -S @wordpress/html-entities' to add it  import/no-extraneous-dependencies
  22:1  error  '@wordpress/url' should be listed in the project's dependencies. Run 'npm i -S @wordpress/url' to add it                      import/no-extraneous-dependencies

✖ 6 problems (6 errors, 0 warnings)

@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality npm Packages Related to npm packages labels Aug 9, 2019
@gziolo gziolo requested review from koke and hypest August 9, 2019 09:12
@gziolo gziolo self-assigned this Aug 9, 2019
@gziolo gziolo added [Type] Bug An existing feature does not function as intended Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Aug 9, 2019
@koke
Copy link
Contributor

koke commented Aug 12, 2019

When I run npm install I see this warning:

npm WARN react-native@0.60.0 requires a peer of react@16.8.6 but none is installed. You must install peer dependencies yourself.

I thought there was something wrong with how the dependencies are set up but updating react to 16.8.6 in @wordpress/element and the main package.json gets rid of the warning

@gziolo
Copy link
Member Author

gziolo commented Aug 12, 2019

Next time, we upgrade react-native we should ensure it uses the correct version of react then :)

Yes, all you need it to update those dependencies to the more recent versions.

By the way, we are working on updating react to v16.9.0.

@gziolo
Copy link
Member Author

gziolo commented Oct 24, 2019

A few notes:

  • @wordpress/components shouldn't depend on @wordpress/data or @wordpress/blocks - the usage should be updated to avoid it
  • @wordpress/format-library is imported inside @wordpress/edit-post - I'm wondering why it is handled differently than in the web version - it should be aligned
  • in @wordpress/rich-text shouldn't depend on @wordpress/blocks, I see why @wordpress/html-entities could be used, but why does it need @wordpress/url?

@gziolo
Copy link
Member Author

gziolo commented Oct 24, 2019

There is also this question of whether react-native should be inlined into @wordpress/element similar to what we did with react-dom. This way we wouldn't have to repeat imports for react-native in all the places and keep the version in sync.

@hypest
Copy link
Contributor

hypest commented Oct 24, 2019

By the way, we are working on updating react to v16.9.0.

We have wordpress-mobile/gutenberg-mobile#1450 in a good place at the moment (need to make sure the iOS build works fine too) which will bring the RN and React upgrades on board.

@gziolo
Copy link
Member Author

gziolo commented Nov 28, 2019

I'm closing it for now. It should be revisited once the Gutenberg mobile project is moved into this repository.

@gziolo gziolo closed this Nov 28, 2019
@gziolo gziolo deleted the update/dependencies-react-native branch November 28, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) npm Packages Related to npm packages [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants