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

Use import type in typescript for all imports #4029

Closed
wants to merge 1 commit into from

Conversation

tomgasson
Copy link
Contributor

@tomgasson tomgasson commented Jul 26, 2022

This makes the following change to typescript artifacts:

-import { ConcreteRequest, Query } from "relay-runtime";
+import type { ConcreteRequest, Query } from "relay-runtime";
 import type { FragmentRefs } from "relay-runtime";

Using the existing useImportTypeSyntax config.

As stated in the config, the import type syntax introduced in Typescript version 3.8 and prevents warnings from importsNotUsedAsValues. This setting also assists naive bundlers from taking a dependency on relay-runtime, when only the types are needed.

I kept this simple. Other paths for implementation would be

  • There's a mix of distinct typegen printers e.g. compiler/crates/relay-typegen/src/typescript.rs, and the inline use of conditionals on the language directly in compiler/crates/relay-compiler/src/artifact_content/content.rs. Consolidating these would allow sharing the existing write_import_type function
  • The imports could instead be consolidated and printed together, rather than potentially having multiple imports from the same file as it is now

Copy link
Contributor

@alunyov alunyov left a comment

Choose a reason for hiding this comment

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

Thank you for PR! This looks good to me!

Can you add a test for these changes?

@tomgasson tomgasson force-pushed the use_import_type_syntax branch 2 times, most recently from 412acee to 91df62e Compare July 29, 2022 01:27
js_module_format: JsModuleFormat::Haste,
typegen_config: TypegenConfig {
language: TypegenLanguage::TypeScript,
use_import_type_syntax: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only difference to the typescript fixtures. I wasn't able to use the %project_config% form of specifying the config in each, because that ends up in a dependency cycle.

The fixtures in relay_codegen also aren't quite appropriate, because they only check the metadata content, not the entire file, and I need to check the imports

Those "other paths for implementation" mentioned would cover where this is done, but would be a significant reshuffle of code

@tomgasson tomgasson requested a review from alunyov July 29, 2022 01:32
@facebook-github-bot
Copy link
Contributor

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@tomgasson tomgasson force-pushed the use_import_type_syntax branch 4 times, most recently from 1ddd84f to 05592c8 Compare August 3, 2022 01:49
@facebook-github-bot
Copy link
Contributor

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@tomgasson tomgasson force-pushed the use_import_type_syntax branch from 05592c8 to 33e1a7c Compare August 3, 2022 15:07
@tomgasson
Copy link
Contributor Author

Removed the missing newline at EOF, because I'm guessing that was the internal lint warning

@alunyov
Copy link
Contributor

alunyov commented Aug 3, 2022

Hmm, @tomgasson for some reason this breaks one of the internal typescript tests (we should probably move these to OSS)

It removes the import type of ConcreteRequest and Query...

/* eslint-disable */
 // [@ts](https://www.internalfb.com/intern/profile/ts)-nocheck
 
-import { ConcreteRequest, Query } from 'relay-runtime';

@tomgasson
Copy link
Contributor Author

tomgasson commented Aug 3, 2022

Hmmm. Are those tests using use_import_type_syntax? Is the output after stripping types / building typescript artifacts? That's kind of the purpose of this change - to not have a value import in the code actually sent to browsers

It's a bit hard to see how this change would remove something if it's only the relay generated output

@tomgasson tomgasson force-pushed the use_import_type_syntax branch from 33e1a7c to 22c8e13 Compare September 19, 2022 04:50
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