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

Split types into their own imports / exports #41285

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

sgomes
Copy link
Contributor

@sgomes sgomes commented Apr 20, 2020

There are currently a number of warnings in the build process, related to how composite-checkout is importing and exporting TypeScript types.

This PR fixes that, by importing and exporting types separately from actual JS constructs.

It also prevents a JS file from attempting to export TS types.

Note that this PR includes a temporary directive to disable an ESLint rule, because of an ongoing bug in its plugin (eslint-plugin-import) with TS 3.8 (see this bug).

Changes proposed in this Pull Request

  • Split types into their own imports / exports

Testing instructions

  • Build calypso locally
  • Ensure all the warnings about missing exports in composite-checkout are gone.
  • Ensure composite-checkout/wpcom-related functionality still works?

This PR mostly fixes #41113, with one warning (in an external package) remaining.

Also prevents JS file from attempting to export TS types.
@sgomes sgomes added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build [Feature] Checkout The checkout screen and process for purchases made on WordPress.com. labels Apr 20, 2020
@sgomes sgomes requested review from sirbrillig, nbloomf and a team April 20, 2020 15:53
@matticbot
Copy link
Contributor

@sgomes sgomes changed the title Splits types into their own imports / exports Split types into their own imports / exports Apr 20, 2020
@matticbot
Copy link
Contributor

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~109 bytes removed 📉 [gzipped])

name      parsed_size           gzip_size
checkout       -404 B  (-0.0%)     -109 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@sirbrillig sirbrillig left a comment

Choose a reason for hiding this comment

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

Looks good to me! Tested composite checkout and nothing appears broken.

Thank you for doing this!

@sirreal
Copy link
Member

sirreal commented Apr 21, 2020

Should we explore the importsNotUsedAsValues compiler option? (set to "error")

Specify emit/checking behavior for imports that are only used for
types. "remove" and "preserve" specify whether to emit unused imports for side effects,
and "error" enforces that imports used only for types are written with import type.

I've been reluctant to start using type imports because I'd read that it will mostly be unnecessary, however I believe that's true with the TypeScript compiler (tsc), but not so with the babel compiler we're currently using. In cases where we import both types and values, do we still get warnings?

@sgomes
Copy link
Contributor Author

sgomes commented Apr 21, 2020

In cases where we import both types and values, do we still get warnings?

@sirreal It isn't clear to me whether the errors this PR is trying to fix are caused by mixed imports or mixed exports. One error that is clear, however, is that there is a JS file trying to export TS types, which shouldn't happen.

Now, the reason I opted for separating things into import and import type (as well as export and export type) is for a matter of clarity. It makes it immediately obvious what is a type and what isn't, which helps prevent errors like the JS file trying to export types.

If a TS file exports both types and JS constructs with the same export statement, it isn't immediately obvious what a JS file can safely consume and re-export, particularly if those are already re-exports in the TS file, as was the case here!

For those reasons, I would vote for always keeping types and JS constructs separate, in their own imports/exports.

@sirreal
Copy link
Member

sirreal commented Apr 21, 2020

I would vote for always keeping types and JS constructs separate.

I'm OK with trying this direction.

@sgomes sgomes merged commit 837374d into master Apr 21, 2020
@sgomes sgomes deleted the fix/composite-checkout-import-bugs branch April 21, 2020 10:20
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build [Feature] Checkout The checkout screen and process for purchases made on WordPress.com.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript warnings about exports not found
4 participants