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

fix: set baseUrl if none is provided #386

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

lachieh
Copy link
Contributor

@lachieh lachieh commented Oct 1, 2024

If paths is set in TS compilerOptions, then as of TS4.1, the TS compiler will ensure that the baseUrl is set to the current working directory.

This PR aligns the plugin to the compiler behavior.

Related to #330

@lachieh
Copy link
Contributor Author

lachieh commented Oct 5, 2024

@qmhc any chance to get this one merged? #330 isn't quite resolved but this will fix it.

@qmhc
Copy link
Owner

qmhc commented Oct 8, 2024

I found that the reproduction of #330 has baseUrl in its tsconfig.json, but the output still is incorrect.

@lachieh
Copy link
Contributor Author

lachieh commented Oct 8, 2024

That's a good point actually, it seems that the problem from the original description is different to the other issues posted in that thread. This PR adds support for leaving out the basePath which will fix the problem mentioned in this comment, but it doesn't entirely solve #330.

@lachieh
Copy link
Contributor Author

lachieh commented Oct 8, 2024

Ok, I added a failing test which seems to accurately show the error. I'll take a look in the next couple of days unless someone else wants to PR to my branch.

@lachieh lachieh force-pushed the fix/add-tsconfig-baseurl-default branch 4 times, most recently from 6603be6 to bf398a0 Compare October 8, 2024 14:30
@lachieh lachieh force-pushed the fix/add-tsconfig-baseurl-default branch from bf398a0 to 3938ac6 Compare October 8, 2024 14:32
Copy link
Contributor Author

@lachieh lachieh left a comment

Choose a reason for hiding this comment

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

Had some time this morning. New test is passing 👍🏻

src/utils.ts Outdated Show resolved Hide resolved
Comment on lines +128 to +134
{
// https://github.com/qmhc/vite-plugin-dts/issues/330
description: 'wildcard alias at root level with relative import',
filePath: './src/components/Sample/index.ts',
content: 'import { Sample } from "./Sample";',
output: "import { Sample } from './Sample';\n"
},
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 added test case that was failing initially. The path was incorrectly being adjusted to ../../Sample because the path replacement pattern /^(.+)/ generated by the '*' path alias was too eagerly matching the relative path and making it absolute.

Comment on lines 237 to +239
expect(transformCode(options('import { TestBase } from "./test";')).content).toEqual(
"import { TestBase } from './test';\n"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Git is a little confused here because of the added tests, but this is the diff for the changed test:

-    expect(transformCode(options('import { TestBase } from "./test";')).content).toEqual(
-      "import { TestBase } from './utils/test';\n"
-    )
+    expect(transformCode(options('import { TestBase } from "./test";')).content).toEqual(
+      "import { TestBase } from './test';\n"
+    )

Essentially, relative paths should stay relative and I had previously made this test incorrect.

Comment on lines +241 to +251
expect(transformCode(options('import { TestBase } from "test";')).content).toEqual(
"import { TestBase } from './utils/test';\n"
)

expect(transformCode(options('import { TestBase } from "test.path";')).content).toEqual(
"import { TestBase } from './utils/test.path';\n"
)

expect(transformCode(options('import { TestBase } from "./test.path";')).content).toEqual(
"import { TestBase } from './test.path';\n"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the new tests cases to cover the correct behavior.

Comment on lines 250 to 254
it('test: getTsLibFolder', () => {
const root = normalizePath(resolve(__dirname, '..'))
const entryRoot = resolve(root, 'src')

expect(getTsLibFolder({ root, entryRoot })).toMatch(/node_modules\/typescript$/)
expect(getTsLibFolder()).toMatch(/node_modules\/typescript$/)

expect(existsSync(getTsLibFolder({ root, entryRoot }) || '')).toBe(true)
expect(existsSync(getTsLibFolder() || '')).toBe(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.

Not related to the main change, but tsc was complaining because getTsLibFolder no longer accepts parameters as of 0621332

@@ -421,7 +421,7 @@ export function parseTsAliases(basePath: string, paths: ts.MapLike<string[]>) {

for (const [pathWithAsterisk, replacements] of Object.entries(paths)) {
const find = new RegExp(
`^${pathWithAsterisk.replace(regexpSymbolRE, '\\$1').replace(asteriskRE, '(?!\\.\\/)([^*]+)')}$`
`^${pathWithAsterisk.replace(regexpSymbolRE, '\\$1').replace(asteriskRE, '(?!\\.{1,2}\\/)([^*]+)')}$`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous pattern (.+) was matching everything when the * pattern was used as a path alias. This matches everything except relative paths (cwd and parent).

@lachieh
Copy link
Contributor Author

lachieh commented Oct 10, 2024

Thanks for the approval! Are you able to merge and release?

@qmhc qmhc merged commit dc3cbfe into qmhc:main Oct 11, 2024
4 checks passed
@lachieh lachieh deleted the fix/add-tsconfig-baseurl-default branch October 11, 2024 16:45
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.

2 participants