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

Data Stores: Disable automatic type inclusion #38688

Merged
merged 3 commits into from
Jan 8, 2020

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Jan 7, 2020

Automatically including type definitions can be problematic. Only
include imported types.

See tsconfig types documentation:
https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#types-typeroots-and-types

> @automattic/data-stores@1.0.0-alpha.0 prepare …/packages/data-stores
> tsc --project ./tsconfig.json && tsc --project ./tsconfig-cjs.json

../../node_modules/@types/node/globals.d.ts(139,11): error TS2320: Interface 'NodeRequire' cannot simultaneously extend types 'Require' and 'RequireFunction'.
  Named property 'cache' of types 'Require' and 'RequireFunction' are not identical.
../../node_modules/@types/node/globals.d.ts(139,11): error TS2320: Interface 'NodeRequire' cannot simultaneously extend types 'Require' and 'RequireFunction'.
  Named property 'resolve' of types 'Require' and 'RequireFunction' are not identical.
../../node_modules/@types/node/globals.d.ts(141,11): error TS2320: Interface 'NodeModule' cannot simultaneously extend types 'Module' and 'Module'.
  Named property 'children' of types 'Module' and 'Module' are not identical.
../../node_modules/@types/node/globals.d.ts(141,11): error TS2320: Interface 'NodeModule' cannot simultaneously extend types 'Module' and 'Module'.
  Named property 'parent' of types 'Module' and 'Module' are not identical.
../../node_modules/@types/node/globals.d.ts(141,11): error TS2320: Interface 'NodeModule' cannot simultaneously extend types 'Module' and 'Module'.
  Named property 'require' of types 'Module' and 'Module' are not identical.
../../node_modules/@types/react-transition-group/node_modules/@types/react/index.d.ts(2891,14): error TS2300: Duplicate identifier 'LibraryManagedAttributes'.
../../node_modules/@types/react/index.d.ts(2852,14): error TS2300: Duplicate identifier 'LibraryManagedAttributes'.

Orinially reported by @jsnajdr

Testing instructions

  • npm run distclean && npm run update-deps will error on master (like above) but will correctly build on this branch.
  • npx lerna run prepare --scope='@automattic/data-stores' is the specific command that would fail

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build labels Jan 7, 2020
@sirreal sirreal requested review from jsnajdr and a team January 7, 2020 13:53
@sirreal sirreal self-assigned this Jan 7, 2020
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

Looks like a good solution 👍

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 7, 2020
Automatically including type definitions can be problematic. Only
include imported types.

See tsconfig types documentation:
https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#types-typeroots-and-types
@sirreal sirreal force-pushed the fix/no-auto-include-types-data-stores branch from 1581b1e to 7e6499f Compare January 8, 2020 11:44
@sirreal
Copy link
Member Author

sirreal commented Jan 8, 2020

I had to apply the fix to all the tsconfigs in use as well as one additional change. This should be working well now.

@@ -1,4 +1,4 @@
require( '@babel/polyfill' );
import '@babel/polyfill';
Copy link
Member Author

Choose a reason for hiding this comment

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

@nbloomf This seem OK to you?

Comment on lines +5 to +6
"target": "ES5",
"lib": [ "DOM", "ES2015", "ScriptHost" ],
Copy link
Member Author

Choose a reason for hiding this comment

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

@nbloomf This seem OK to you?

My understanding is that target ES5 changes compiled output, while lib allows all ES2015 features, e.g. Promise, Set.

@sirreal sirreal requested a review from nbloomf January 8, 2020 11:51
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 8, 2020
@sirreal sirreal merged commit d265dcd into master Jan 8, 2020
@sirreal sirreal deleted the fix/no-auto-include-types-data-stores branch January 8, 2020 12:56
@sgomes sgomes mentioned this pull request Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants