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

Vite config: import aliases, eslint config #142

Merged
merged 6 commits into from
Jan 11, 2024
Merged

Vite config: import aliases, eslint config #142

merged 6 commits into from
Jan 11, 2024

Conversation

ioay
Copy link
Contributor

@ioay ioay commented Jan 9, 2024

Closes: #140

Added @~ prefix to local aliases. Thanks to this, we distinguish local files from external packages.

@ioay ioay self-assigned this Jan 9, 2024
dapp/tsconfig.json Outdated Show resolved Hide resolved
@ioay ioay marked this pull request as ready for review January 9, 2024 11:13
@nkuba nkuba requested a review from kkosiorowska January 9, 2024 11:15
@ioay ioay requested a review from nkuba January 9, 2024 11:16
@nkuba nkuba requested a review from r-czajkowski January 9, 2024 11:17
@nkuba
Copy link
Member

nkuba commented Jan 10, 2024

Added @~ prefix to local aliases. Thanks to this, we distinguish local files from external packages.

In this PR in addition to defining aliases, we're restructuring the code by moving some content to the assets directory. All the changes are included in the commit c5681ae.

What do you think about splitting aliases and asset changes into two separate commits (ideally two separate PRs)?
We should justify in the PR and commit descriptions why we're introducing such changes, for example, it's unclear to me why we're moving fonts to assets, but leaving icons in the static directory.

Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

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

I wonder if it is simpler to use the @ alias instead of @~. I understand that we want to distinguish local files from external packages. But I wonder if it is really necessary. 🤔

@ioay
Copy link
Contributor Author

ioay commented Jan 10, 2024

I wonder if it is simpler to use the @ alias instead of @~. I understand that we want to distinguish local files from external packages. But I wonder if it is really necessary. 🤔

I'm open to discussing the naming convention, but just @ is generally a bad approach by using it in external packages and it may also introduce an error in the sorting of imports in IDE. I used @~ because:

  • @~ sorts imports perfectly
  • ~ is commonly known as the home path in iOS system, so @~ is unique and logical.

@r-czajkowski what are your preferences?

@ioay
Copy link
Contributor Author

ioay commented Jan 10, 2024

Added @~ prefix to local aliases. Thanks to this, we distinguish local files from external packages.

In this PR in addition to defining aliases, we're restructuring the code by moving some content to the assets directory. All the changes are included in the commit c5681ae.

What do you think about splitting aliases and asset changes into two separate commits (ideally two separate PRs)? We should justify in the PR and commit descriptions why we're introducing such changes, for example, it's unclear to me why we're moving fonts to assets, but leaving icons in the static directory.

I was changing aliases everywhere, so I moved this one too. It's just for fonts used in one file.
Icons are not graphic .svg files. I added more details related to this in another PR comment: #132 (comment)

@ioay ioay requested a review from kkosiorowska January 10, 2024 12:32
@nkuba
Copy link
Member

nkuba commented Jan 10, 2024

I wonder if it is simpler to use the @ alias instead of @~. I understand that we want to distinguish local files from external packages. But I wonder if it is really necessary. 🤔

I'm open to discussing the naming convention, but just @ is generally a bad approach by using it in external packages and it may also introduce an error in the sorting of imports in IDE. I used @~ because:

* `@~` sorts imports perfectly

* `~` is commonly known as the home path in iOS system, so `@~` is unique and logical.

@r-czajkowski what are your preferences?

@ alone, e.g. @assets may be confusing as npm pacakges use @ prefix for scopes.
Are you aware of any formal recommendations for local aliases naming convention?

What about # prefix defined recently by Node?

@r-czajkowski
Copy link
Contributor

I wonder if it is simpler to use the @ alias instead of @~. I understand that we want to distinguish local files from external packages. But I wonder if it is really necessary. 🤔

I'm open to discussing the naming convention, but just @ is generally a bad approach by using it in external packages and it may also introduce an error in the sorting of imports in IDE. I used @~ because:

* `@~` sorts imports perfectly

* `~` is commonly known as the home path in iOS system, so `@~` is unique and logical.

@r-czajkowski what are your preferences?

What about @acre-dapp/*? We can also use # instead of @, as Kuba mentioned, to indicate it is an internal path.

@ioay
Copy link
Contributor Author

ioay commented Jan 10, 2024

I wonder if it is simpler to use the @ alias instead of @~. I understand that we want to distinguish local files from external packages. But I wonder if it is really necessary. 🤔

I'm open to discussing the naming convention, but just @ is generally a bad approach by using it in external packages and it may also introduce an error in the sorting of imports in IDE. I used @~ because:

* `@~` sorts imports perfectly

* `~` is commonly known as the home path in iOS system, so `@~` is unique and logical.

@r-czajkowski what are your preferences?

What about @acre-dapp/*? We can also use # instead of @, as Kuba mentioned, to indicate it is an internal path.

Agreed, as I mentioned previously - just @ is a bad option because of external packages. It's why I recommend @~. @acre-dapp looks like a node-module package. If @~ is not good for you what do you think about # (previously mentioned by @nkuba ) or $ ?

dapp/tsconfig.json Outdated Show resolved Hide resolved
@ioay ioay force-pushed the vite-config-aliases branch 2 times, most recently from aa8529e to 6c29635 Compare January 11, 2024 13:39
@ioay ioay force-pushed the vite-config-aliases branch from 6c29635 to 2cdb450 Compare January 11, 2024 13:41
@ioay ioay requested a review from nkuba January 11, 2024 13:44
dapp/package.json Outdated Show resolved Hide resolved
dapp/vite.config.ts Show resolved Hide resolved
@ioay ioay requested a review from nkuba January 11, 2024 15:16
@ioay ioay requested a review from nkuba January 11, 2024 15:42
@nkuba nkuba enabled auto-merge January 11, 2024 16:08
@nkuba nkuba merged commit 09b5ff5 into main Jan 11, 2024
11 checks passed
@nkuba nkuba deleted the vite-config-aliases branch January 11, 2024 16:09
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.

Vite config for import aliasses
4 participants