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 tsconfig alias regression #6617

Merged
merged 6 commits into from
Mar 22, 2023
Merged

Fix tsconfig alias regression #6617

merged 6 commits into from
Mar 22, 2023

Conversation

MoustaphaDev
Copy link
Member

Changes

Fix #6613

Testing

Check that Typescript file without .ts extension can be imported

Docs

N/A, bug fix only

@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2023

🦋 Changeset detected

Latest commit: f63a92b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Mar 21, 2023
if (id.startsWith('.') || id.startsWith('/')) return;

// Handle baseUrl mapping for non-relative and non-root imports.
// Since TypeScript only applies `baseUrl` autocompletions for files that exist
// in the filesystem only, we can use this heuristic to skip resolve if needed.
const resolved = path.posix.join(resolvedBaseUrl, id);
if (fs.existsSync(resolved)) {
Copy link
Member Author

@MoustaphaDev MoustaphaDev Mar 21, 2023

Choose a reason for hiding this comment

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

When importing TypeScript files without the .ts extension, this condition was always false, as /path/to/ts-file doesn't exist.

Thought about adding a check for .ts files starting with the resolved id to make the change minimal but realized that there might be other file types that could be resolved when not specifying the extension (maybe via vite plugins pushed before the ts/js resolver? Not sure)

Copy link
Member

Choose a reason for hiding this comment

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

Great call!

packages/astro/test/alias-tsconfig.test.js Outdated Show resolved Hide resolved
@MoustaphaDev
Copy link
Member Author

MoustaphaDev commented Mar 22, 2023

There's a weird path issue occuring on Windows, I'll debug this tomorrow on my windows machine

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Windows passes now! It was because it checked for absolute imports via .startsWith('/') which doesn't match on Windows. I've changed that to path.isAbsolute.

@bluwy bluwy merged commit 38e6ec2 into main Mar 22, 2023
@bluwy bluwy deleted the moustapha/fix-alias-regression branch March 22, 2023 06:10
@astrobot-houston astrobot-houston mentioned this pull request Mar 22, 2023
@MoustaphaDev
Copy link
Member Author

Thanks for jumping in @bluwy!

bholmesdev added a commit that referenced this pull request Mar 23, 2023
bholmesdev added a commit that referenced this pull request Mar 23, 2023
* Revert "Fix tsconfig alias regression (#6617)"

This reverts commit 38e6ec2.

* chore: changeset

* fix: add back fs

* chore: remove stray console log

* Revert "Support tsconfig aliases in styles (#6566)"

This reverts commit ea9b3dd.

* chore: add note on css style aliases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal server error: Failed to resolve import "lib/getClient" from...
3 participants