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.

Sign up for GitHub

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

Jump to bottom

add support for Extension Preference in import #25073

Closed
wants to merge 2 commits into from
Closed

add support for Extension Preference in import #25073

wants to merge 2 commits into from

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Jun 19, 2018

Fixes #24779

mhegazy
mhegazy reviewed Jun 19, 2018
View reviewed changes
src/compiler/moduleSpecifiers.ts Outdated
@@ -3,11 +3,12 @@
namespace ts.moduleSpecifiers {
export interface ModuleSpecifierPreferences {
importModuleSpecifierPreference?: "relative" | "non-relative";
includingJsExtensionOnAutoImports?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not always .js, it can also be .jsx. so the name of the flag should not include Js. also this also applies to all LS operations and not only auto-imports for completions. i would just call it includeExtensionInImports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in src/compiler/moduleSpecifiers L400 removeExtensionAndIndexPostFix
i saw it only handle the .js Extension, should i make a change for that to support jsx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also a minor problem in importNameCodeFix_jsExtension test case:
why could it import a file with .js extension even if that was a ts file,

Copy link

Choose a reason for hiding this comment

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

That was supported intentionally in #8895.

mhegazy
mhegazy reviewed Jun 19, 2018
View reviewed changes
src/compiler/moduleSpecifiers.ts Outdated
function usesJsExtensionOnImports({ imports }: SourceFile): boolean {
return firstDefined(imports, ({ text }) => pathIsRelative(text) ? fileExtensionIs(text, Extension.Js) : undefined) || false;
function usesJsExtensionOnImports({ imports }: SourceFile, preferences: ModuleSpecifierPreferences): boolean {
return firstDefined(imports, ({ text }) => pathIsRelative(text) ? fileExtensionIs(text, Extension.Js) : undefined) || !!preferences.includingJsExtensionOnAutoImports;
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to handle .jsx here as well.

also please add a test to it.

Copy link

Choose a reason for hiding this comment

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

I think an explicit (non-undefined) preference should always take priority over looking through the source file for an existing import.

@mhegazy mhegazy requested a review from a user June 19, 2018 17:16
@mhegazy
Copy link
Contributor

mhegazy commented Jun 19, 2018

@andy-ms can you please review

@Kingwl Kingwl force-pushed the jsExtensionPreference branch 2 times, most recently from 04f3e1a to 4d57419 Compare June 22, 2018 17:08
@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 22, 2018 •
edited
Loading

i have try track the issue to understand that why the js or jsx extension been append after filename
IMO, that output the file extension for other packager or runtime,
it need an artificial extension(Preference) rather than the extension that itself has,
i have a try that take extension name by itself file name and i drop that
currently, i take a Preference that allow user set the extension (js or jsx)

@mhegazy @andy-ms could you give some suggestion please😢

@Kingwl Kingwl changed the title add support for jsExtension Preference on auto import add support for Extension Preference in import Jun 22, 2018
ghost
ghost reviewed Jun 22, 2018
View reviewed changes
src/compiler/moduleSpecifiers.ts Outdated
@@ -3,11 +3,12 @@
namespace ts.moduleSpecifiers {
export interface ModuleSpecifierPreferences {
importModuleSpecifierPreference?: "relative" | "non-relative";
includeExtensionInImports?: Extension.Js | Extension.Jsx;
}

// Note: fromSourceFile is just for usesJsExtensionOnImports
Copy link

@ghost ghost Jun 22, 2018 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

From #25074 (comment) -- maybe we shouldn't take fromSourceFile as input and just use includeExtensionInImports for that.

@ghost ghost mentioned this pull request Jun 22, 2018
Merged
ghost
ghost reviewed Jun 22, 2018
View reviewed changes
src/compiler/moduleSpecifiers.ts Outdated
@@ -3,11 +3,12 @@
namespace ts.moduleSpecifiers {
export interface ModuleSpecifierPreferences {
importModuleSpecifierPreference?: "relative" | "non-relative";
includeExtensionInImports?: Extension.Js | Extension.Jsx;
Copy link

Choose a reason for hiding this comment

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

It seems wrong for this to be configurable as always .js or .jsx. I think the right policy is: if the file you're importing from is a .ts or .js file you should import with .js (if the setting is on), if the file is .tsx or .jsx you should import with .jsx (if the setting is on). Then all the user has to set is: yes add extensions, or no don't add extensions.

Copy link

Choose a reason for hiding this comment

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

Actually, .tsx files are typically compiled to .js, right? So it would still make sense to use a .js import always? @mhegazy

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, .tsx files are typically compiled to .js, right?

depends on the setting.. --JSX preserve will result in .jsx, --JSX react/--JSX react-native will result in .js.

@Kingwl Kingwl force-pushed the jsExtensionPreference branch from d3d34b9 to 607daf9 Compare June 22, 2018 20:11
@Kingwl Kingwl mentioned this pull request Jun 25, 2018
Closed
@mhegazy
Copy link
Contributor

mhegazy commented Jul 2, 2018

@andy-ms can you take another look.

@mhegazy mhegazy requested a review from weswigham July 3, 2018 22:21
weswigham
weswigham reviewed Jul 3, 2018
View reviewed changes
src/compiler/moduleSpecifiers.ts Outdated
return addJsExtension
? noExtension + ".js"
const actualExtension = tryGetActualExtension(fileName, compilerOptions);
return (preferences.includeExtensionInImports && actualExtension)
Copy link
Member

Choose a reason for hiding this comment

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

If includeExtensionInImports is undefined (so not explicitly set), we should still be doing what the usesJsExtensionOnImports function did - context based method of determining which way we should go based on what's already there.

@Kingwl Kingwl force-pushed the jsExtensionPreference branch 2 times, most recently from b449ff0 to 29d7c73 Compare July 6, 2018 09:35
@shrinktofit
Copy link

How to config the tsconfig?

@Kingwl Kingwl force-pushed the jsExtensionPreference branch from 29d7c73 to b24489f Compare July 25, 2018 10:12
@magic-akari magic-akari mentioned this pull request Jul 31, 2018
Closed
@ghost ghost mentioned this pull request Aug 6, 2018
Merged
@Kingwl
Copy link
Contributor Author

Kingwl commented Aug 29, 2018

@andy-ms should i close this?

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.2 milestone Sep 17, 2018
@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 29, 2018

@RyanCavanaugh is the milestone means i need to update the branch?🤦🏻‍♂️

@ghost ghost assigned sheetalkamat Nov 17, 2018
@Kingwl
Copy link
Contributor Author

Kingwl commented Dec 12, 2018

update soon🚲

@Kingwl
Copy link
Contributor Author

Kingwl commented Jan 22, 2019

Seems already implements

@Kingwl Kingwl closed this Jan 22, 2019
@Kingwl Kingwl deleted the jsExtensionPreference branch June 18, 2019 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhegazy mhegazy mhegazy left review comments

@weswigham weswigham weswigham left review comments

Assignees

@sheetalkamat sheetalkamat

Labels
None yet
Projects
None yet
Milestone
TypeScript 3.3
Development

Successfully merging this pull request may close these issues.
7 participants
@Kingwl @mhegazy @shrinktofit @weswigham @DanielRosenwasser @RyanCavanaugh @sheetalkamat