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

feat: replacing aliasing with aliasHQ #15702

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Nike682631
Copy link
Contributor

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Documentation
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

The PR fulfills these requirements:

  • It's submitted to the dev branch (or v[X] branch)
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on an Electron app
  • Any necessary documentation has been added or updated in the docs or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to start a new feature discussion first and wait for approval before working on it)

Other information:

@Nike682631
Copy link
Contributor Author

This pr is not fit for merging yet. The main reason it that although it can do the aliasing via aliasHQ, currently aliasHQ is using a JSON parser that does not recognize comments. An issue has been created in the aliasHQ repo for this. In the meanwhile I'll try to resolve this isssue by forking aliasHQ and making a pr for it. We can proceed according by either

  • publishing our own version of aliasHQ on npm registry with JSON5 parser
  • or if we get a response from aliasHQ maintainer than by making a pr to aliasHQ.

Following was the effort made towards this issue:-

First a research was done on what are good aliasing solutions that can be implemented. Below is a snippet of my feedback before we went for using aliasHQ.


Below are some solutions I found for this

https://github.com/ilearnio/module-alias
Pros
Configuration can be made in package.json so I believe that should be possible to do for both vite and webpack. They have already for webpack. Need to test that for vite
usage seems simple
It's possible to set aliases programmatically rather than adding them in package.json
Cons
This module does not play well with:
Front-end JavaScript code. Module-alias is designed for server side so do not expect it to work with front-end frameworks (React, Vue, ...) as they tend to use Webpack. Use Webpack's resolve.alias mechanism instead.
Jest, which discards node's module system entirely to use it's own module system, bypassing module-alias.
The NCC compiler, as it uses WebPack under the hood without exposing properties, such as resolve.alias. It is not something they wish to do.
https://github.com/davestewart/alias-hq
Pros
Seems easy to use as well and provides one liner integration.
Integrations for both Vite and webpack
Benefits mentioned as follows
If you are already using aliases:
The Alias API simplifies your tooling with a single config file and one-liner integrations
If you are thinking about using aliases:
The Alias CLI migrates your project by configuring your paths and rewriting your imports
You can configure and migrate any project in less than a minute by:
installing the package
running the CLI
following the prompts
cons
Nothing specific I could notice
I don't find any other good solutions for this as of yet

Perosonal Recommendations: I referred to this article and alias hq seems like an overall good and managed option. Will add more to this if I find something else.

I tried to dive a bit deeper into the nuxt repo. It seems that nuxt is manually resolving the paths for aliasing.

vite

webpack

pros
We might not need any additional module like aliashq to set this up and we can try following the approach being taken in nuxt.
cons
I feel this approach might be a bit lengthier to implement.
Ultimately this approach just seems to take a different direction as aliashq seems to use ts/jsconfig to deal with aliasing while with nuxt's approach is to use webpack or viteconfig to resolve this. This way we can kinda say these two solutions are a little similar in implementation. While at one hand using aliashq we might ask for support in implementing it in quasar we can probably not expect any external support like that in the usual webpack or vite solution. Both approaches seem feasible though so we can probably take any of them.


@IlCallo IlCallo marked this pull request as draft April 13, 2023 07:07
@Nike682631 Nike682631 marked this pull request as ready for review April 25, 2023 12:58
@rstoenescu
Copy link
Member

Hi,

This PR is not acceptable into the core because it adds yet another dependency with little value to the majority of our users. But the main problem is that in its current form it breaks the expected pre-configured aliases.

However, you can configure alias-hq to use it in your own projects with no problem.

@rstoenescu rstoenescu closed this Apr 26, 2023
@IlCallo
Copy link
Member

IlCallo commented Apr 26, 2023

@Nike682631 meant to ask my review, he forgot to ping me
This feature is similar to what Nuxt does with its aliases, but Nuxt take the other route around, by defining aliases into nuxt.config.js and dynamically generating the jsconfig/tsconfig

We can switching to Nuxt way of doing so, using quasar.config.js as a source of truth, but that would probably be backward incompatible due to our current setup, based on a tsconfig preset served by q-vite or q-webpack packages


This PR is not acceptable into the core because it adds yet another dependency with little value to the majority of our users

The PR main aim is to ease the project maintenance
We choose a source of truth for aliases from which then configure all other tools aliases (VSCode, TS, Webpack/Vite, testing frameworks, etc), so that we shouldn't bother updating multiple places for the same
As a byproduct, users will automatically have aliases configured for bundlers, compilers, etc even for custom aliases they may define

This is also a first step towards refreshing Capacitor/Electron support, since their usage with TS (and IDE in general) is clunky due to bundlers aliases not being in sync with TS ones

We're already using AliasHQ into Jest testing AE to automatically replicate a project aliases into Jest

But the main problem is that in its current form it breaks the expected pre-configured aliases.

The change is meant to be fully backwards compatible and transparent to users, which shouldn't know which package we're using under the hood, I just didn't have the chance to review and help iterate on it yet
It's meant to be transparent also to allow an easy revert later on or to change the way we implement this feature if we hit too many limitations while working on it

@davestewart
Copy link

davestewart commented Apr 26, 2023

This PR is not acceptable into the core because it adds yet another dependency with little value to the majority of our users. But the main problem is that in its current form it breaks the expected pre-configured aliases.

Hey @rstoenescu!

Alias HQ author here; just catching up on this PR after answering some questions on it by @Nike682631 and @IlCallo .

Totally respect your decision regarding using the lib, was just wondering if you have time to clarify your comment regarding "breaking pre-configured aliases" ?

Would be good for me to know the use cases where it may not be useful / appropriate / compatible.

🙏

@rstoenescu
Copy link
Member

@davestewart

Hi there! Was referring to the fact that our current list of aliases are replaced by alias: hq.get('rollup'). If I am missing something, please do tell me.

I'm going to reopen this and would love to have a chat with you on Discord over AliasHQ. Maybe I closed this too early.

@rstoenescu rstoenescu reopened this Apr 27, 2023
@davestewart
Copy link

davestewart commented Apr 27, 2023

Cool! I messaged you (rstoenescu#1511) on the Quasar Discord 🙂

@IlCallo
Copy link
Member

IlCallo commented Apr 27, 2023

Was referring to the fact that our current list of aliases are replaced by alias: hq.get('rollup')

As long as jsconfig and tsconfig aliases are defined, AliasHQ is capable of keeping them in sync automatically
I should be able to review and test it next week, I'll ping you when/if we reach a "mergeable" state and ping you for an additional review

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.

4 participants