Skip to content

feat: Use ~/* import alias #1191

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 7 commits into from
Feb 18, 2023
Merged

feat: Use ~/* import alias #1191

merged 7 commits into from
Feb 18, 2023

Conversation

bai
Copy link
Contributor

@bai bai commented Feb 10, 2023

✅ Checklist

  • I have followed every step in the contributing guide (updated 2022-10-06).
  • The PR title follows the convention we established conventional-commit
  • I performed a functional test on my final commit

Changelog

Use ~/* import alias like the one recently added to Next.js.


💯

@changeset-bot
Copy link

changeset-bot bot commented Feb 10, 2023

🦋 Changeset detected

Latest commit: 9312177

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

This PR includes changesets to release 1 package
Name Type
create-t3-app Patch

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

@vercel
Copy link

vercel bot commented Feb 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
create-t3-app ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 17, 2023 at 5:45PM (UTC)

@github-actions github-actions bot added 📌 area: cli Relates to the CLI 📌 area: t3-app Relates to the generated T3 App labels Feb 10, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2023

Running Lighthouse audit...

Copy link
Member

@c-ehrlich c-ehrlich left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. We're gonna need a bit of time to think about this one. Import aliases are cool but there are still some issues related to them as well, and different users have different needs.

Here are the main possibilities:

  • Keep on not using an alias at all.
  • Use a hardcoded import alias. If so, do we want @/? ~/? something else?
  • Allow the user to define the alias, like create-next-app does. Is this worth the additional complexity?
  • Offer the choice of no alias vs alias in the cli (this feels like it would be very difficult to implement with our current scaffolding setup).

@bai
Copy link
Contributor Author

bai commented Feb 10, 2023

Thanks for feedback. My thinking was that ct3a is opinionated so it'd be fine to have one more opinion. I personally have no preference on whether it's @/ or ~/ or something else as long as it resolves the inconvenience of counting/keeping-in-mind ../'s.

I certainly would prefer not to "Allow the user to define the alias" since it'd add more mental overhead to everyone involved.

@juliusmarminge
Copy link
Member

I certainly would prefer not to "Allow the user to define the alias" since it'd add more mental overhead to everyone involved.

It's not that much overhead, Next does this fairly straightforward:

https://github.com/vercel/next.js/blob/7bbd239fe7e6d72ef467484619b978c11f5fbb70/packages/create-next-app/templates/index.ts#L85-L104

Allowing either with/without aliases would be difficult though

@bai
Copy link
Contributor Author

bai commented Feb 13, 2023

Thanks @juliusmarminge, that looks pretty straightforward - happy to implement this if you think it's fine to have aliases used within ct3a 🙏🏼

@c-ehrlich
Copy link
Member

c-ehrlich commented Feb 13, 2023

I thought about this and I think I'm leaning towards "default to a sensible alias, and let the user override".

I'm not a fan of @ as many packages also use this. My vote for the default would be either ~ (create-next-app uses this) or ~~ (nuxt uses this). Leaning towards ~ as we're more closely related to create-next-app than to nuxt, but don't feel strongly about this either way.

@bai
Copy link
Contributor Author

bai commented Feb 14, 2023

@c-ehrlich I wanted to confirm with you something: been meaning to switch from @/ to ~/ and while going through create-next-app, I noticed that they actually use the former by default:

Screenshot 2023-02-14 at 05 25 43

Since I have no preference on the alias and you mentioned that "we're more closely related to create-next-app", I wanted to double-check with you that you're preferring ~/ instead of create-next-app's default? 🙏🏼

@juliusmarminge
Copy link
Member

Since I have no preference on the alias and you mentioned that "we're more closely related to create-next-app", I wanted to double-check with you that you're preferring ~/ instead of create-next-app's default? 🙏🏼

Yes ~/* by default

@bai
Copy link
Contributor Author

bai commented Feb 14, 2023

Alright, this seems to be working as expected.

After packages are installed, I find-and-replace the default (~/) with the alias entered by a user. I initially used the glob package like create-next-app does but then replaced that with a function.

Could you please take a look? And apologies for a bit of commit spam, this is my first contribution to ct3a and I'm very likely doing something incorrectly 🙏🏼

@bai bai changed the title feat: Use @/* import alias feat: Use ~/* import alias Feb 14, 2023
Copy link
Member

@c-ehrlich c-ehrlich left a comment

Choose a reason for hiding this comment

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

Setting up the alias works and the regexes fix all the incorrect input i could think of. Also tried a couple different apps, everything works great.

The only issue I have is that in VSCode, both before and after adding javascript.preferences.importModuleSpecifier": "non-relative" to settings, autoimports still prefer relative paths. Anyone know what's up with that?

image

@bai
Copy link
Contributor Author

bai commented Feb 18, 2023

@c-ehrlich adding "typescript.preferences.importModuleSpecifier": "non-relative" fixes the issue for me.

Note that there are two settings:

Screenshot 2023-02-18 at 20 33 23

@c-ehrlich
Copy link
Member

@c-ehrlich adding "typescript.preferences.importModuleSpecifier": "non-relative" fixes the issue for me.

Note that there are two settings:

Screenshot 2023-02-18 at 20 33 23

oh yeah makes sense that it's not in javascript 😅

nonetheless this is not great dx as we are now making everyone use absolute imports, but the default editor setting is to ignore this. i guess this brings us back to the debate of whether we want to ship a .vscode with a bunch of stuff in it, i'm still leaning towards no personally but the reasons to do so seem to keep increasing.

i just checked and create-next-app has the same issue, so at least we're not alone.

let's get this merged, and we can think about the other stuff later

@c-ehrlich c-ehrlich added this pull request to the merge queue Feb 18, 2023
Merged via the queue into t3-oss:next with commit aabb9a4 Feb 18, 2023
@bai bai deleted the absolute branch February 19, 2023 03:34
@bai
Copy link
Contributor Author

bai commented Feb 19, 2023

Thanks for reviews and helping me ship this ❤️

Re .vscode config dir, one more reason to have one would probably be Next.js' TypeScript Plugin that requires configuring VS Code to use local TypeScript SDK. From what I've seen while playing with app dir (experimental-app), create-next-app will actually generate .vscode dir for you.

korawit-dvtx pushed a commit to korawit-dvtx/create-devviantex-nextjs-app-deprecated that referenced this pull request Jun 9, 2024
* feat: Use @/* import alias

* Allow users override import alias

* Change default import alias to ~/

* Run setImportAliases at a later stage

---------

Co-authored-by: Christopher Ehrlich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📌 area: cli Relates to the CLI 📌 area: t3-app Relates to the generated T3 App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants