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

Typescript: Exclude native files #21491

Merged
merged 1 commit into from
Apr 10, 2020
Merged

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Apr 8, 2020

Description

Exclude files ending in .android.js, *.ios.js, and *.native.js from TypeScript lookup and compilation.

This is required for PRs such as #21482 that add types to a package that also has a number of .native.js files, unless we also add types to those files (which would significantly increase the workload needed to get our packages typed). However, that doesn't seem to make a lot of sense, since the native modules don't seem to be run through any sort of TypeScript build chain anyway.

How has this been tested?

npm run build:package-types

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ockham ockham added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Apr 8, 2020
@ockham ockham self-assigned this Apr 8, 2020
@ockham ockham mentioned this pull request Apr 8, 2020
6 tasks
@github-actions
Copy link

github-actions bot commented Apr 8, 2020

Size Change: 0 B

Total Size: 890 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.4 kB 0 B
build/api-fetch/index.js 3.8 kB 0 B
build/autop/index.js 2.59 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.03 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 103 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.23 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/style-rtl.css 7.42 kB 0 B
build/block-library/style.css 7.43 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/blocks/index.js 57.5 kB 0 B
build/components/index.js 196 kB 0 B
build/components/style-rtl.css 16.6 kB 0 B
build/components/style.css 16.5 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.7 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.24 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.1 kB 0 B
build/edit-navigation/index.js 2.75 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/index.js 92.9 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.1 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.18 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 42.8 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 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.95 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/keyboard-shortcuts/index.js 2.3 kB 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/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3 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 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.54 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.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@aduth
Copy link
Member

aduth commented Apr 9, 2020

There was some prior consideration for this in #18942, though loosely based on problems with the initial setup. I'm not sure if there was an explicit goal in having native code typed as well.

#18942 (comment)
#18942 (comment)

cc @sirreal @Tug

@ockham
Copy link
Contributor Author

ockham commented Apr 9, 2020

There was some prior consideration for this in #18942, though loosely based on problems with the initial setup. I'm not sure if there was an explicit goal in having native code typed as well.

#18942 (comment)
#18942 (comment)

cc @sirreal @Tug

Thanks Andrew! I had a video call about this with @sirreal yesterday and got his blessing for this approach 😊 (edit: or rather, this approach was suggested by him)

The problem is that not only are some of those native implementations more complex than their web counterparts (and thus more work to type), they also import 3rd party packages for which typings might be unavailable.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Works for me 👍

@gziolo
Copy link
Member

gziolo commented Apr 10, 2020

We took a similar approach for one of ESLint rules:
https://github.com/WordPress/gutenberg/blob/2a5146f6a078ae4ebfb9b0b806daac17674e7a95/.eslintrc.js#L125L135

@gziolo gziolo merged commit 216c6c4 into master Apr 10, 2020
@gziolo gziolo deleted the update/tsconfig-base-to-exclude-mobile branch April 10, 2020 04:29
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 10, 2020
@sirreal
Copy link
Member

sirreal commented Apr 10, 2020

I had a video call about this with @sirreal yesterday and got his blessing for this approach 😊 (edit: or rather, this approach was suggested by him)

I'll add a bit of detail. We were looking into missing type declarations and came across react-native-svg. It's a package that includes a type declaration, so it was surprising that the types were missing.

When we investigated, it's used here:

import { Svg } from 'react-native-svg';

But it's not declared as a dependency of the @wordpress/primitives package.

It is declared in the gutenberg-mobile project:

https://github.com/wordpress-mobile/gutenberg-mobile/blob/a4896fc225712d7efab3e0ec1182afdc1008a897/package.json#L177

https://github.com/wordpress-mobile/react-native-svg

Are these packages intended to be consumed via npm for react-native projects? If so, I suspect this case is a bug. If not, should the react-native parts of packages even be published?


As far as a TypeScript build is concerned (generating type declarations and type checking projects), I don't think there's any way around having web and react-native projects be independent. They would share some files, but the environment is different in ways that we can't expect a single setup to cover everything, certainly not without making big sacrifices in some areas.

I'd expect the web build to ignore files like we've done here (native, android, ios). A react-native TypeScript project would need to be a different build.

I'm under the impression that gutenberg-mobile is the only consumer of the react-native project and they're not using TypeScript, so we can just focus on web.

cc: @SergioEstevao @Tug

@sirreal sirreal added [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality labels Apr 10, 2020
@SergioEstevao
Copy link
Contributor

I'm under the impression that gutenberg-mobile is the only consumer of the react-native project and they're not using TypeScript, so we can just focus on web.

Funny fact when we started the gb-mobile project we wanted to use TypeScript but didn't move forward because the web project was not using and so the benefits were limited.

Now that it's looks that the web is moving in that direction we can reevaluate that option.

cc @maxme @hypest and @pinarol

@Tug
Copy link
Contributor

Tug commented Apr 10, 2020

Yes, these packages (react-native-*...) are only used for the mobile build, which dependencies are defined in the gutenberg-mobile project package.json as you noticed. There's an ongoing effort to bring the build to gutenberg and that includes having the npm dependencies right.
Indeed it will be a different build, with its own typescript config most probably.

@hypest
Copy link
Contributor

hypest commented Apr 13, 2020

we can reevaluate that option

💯 , let's go for it!

Note, I'm not sure what are the subtasks to get this done for the native mobile codepaths, build pipleline and developer tooling, but I understand that we should have the Monorepo first before trying to introduce TS to the native mobile side of things.

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) [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants