Skip to content

Resolve module specifier relative to moduleFile.originalFileName #32722

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

Merged
merged 4 commits into from
Aug 6, 2019

Conversation

rbuckton
Copy link
Contributor

@rbuckton rbuckton commented Aug 5, 2019

Prior to this fix we would resolve the module specifier relative to the file name of the referenced project's output file, rather than its input file, possibly resulting in an incorrect path.

Fixes #31696

const data = host.vfs.readFileSync("/lib/src/sub-project-2/index.d.ts", "utf8");

// Prior to fixing GH31696 the import below was `import("../../lib/src/common/nonterminal")`
assert.equal(data, `declare const variable: {\n key: import("../common/nominal").Nominal<string, "MyNominal">;\n};\nexport declare function getVar(): keyof typeof variable;\nexport {};\n`);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this can you baseline the change. Try verifyTsbuildOutput You can pass baselineonly option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep this simple. verifyTsbuildOutput seemed to do quite a bit more than was strictly necessary to test this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also emits quite a bit of excess information unrelated to what I'm actually trying to verify, since it emits the entire file system diff.

Copy link
Member

Choose a reason for hiding this comment

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

These kind of verification do not perform well when we change our emit. So baselines are better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've switched to baselines, but still feel that verifyTsbuildOutput was too complex for what I needed to verify.

Copy link
Member

@sheetalkamat sheetalkamat Aug 6, 2019

Choose a reason for hiding this comment

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

I disagree. With just baselineOnly option it just baselines the build. You can specify only initial build and it will baseline only that in a single file. Its like our compiler test runner that has complexities but does baseline outputs well. Having two types of baselines is just confusing and error prone later if we say want to add what happens in incremental scenario or something like that. Also you input file reside as files on disk so its easier to run normal build if needed on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into migrating it to verifyTsbuildOutput and I'm not convinced. The setup to use that function seems unnecessarily complicated and is undocumented. I'd much rather keep the test simple and switch to verifyTsbuildOutput if we ever need any of that added complexity. I can switch to emitting the single file patch using the same path format other tsbuild tests use, but I'd rather keep the test as focused as possible for the time being.

@rbuckton rbuckton requested a review from weswigham August 6, 2019 17:27
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Should we have skipLibCheck on for this test?

@rbuckton rbuckton merged commit d757402 into master Aug 6, 2019
@rbuckton rbuckton deleted the fix31696 branch August 6, 2019 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declaration file has wrong import path for composite projects
3 participants