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

Add options to enable aliases for siblings and subpaths #175

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

chapa
Copy link

@chapa chapa commented Jun 4, 2024

Hello,

This is a continuation of #130 (which has been closed by its author) and closes #133.

This adds 2 options forSubpaths and forSiblings, to enable aliases for these specific cases. I separated them because I wanted only siblings (import from './foo') and not subpaths (import from ./sub/foo) to be converted to aliases.

I took the liberty to refactor a bit because the previous main conditional branches (if (sourcePath |> isParentImport) and if (!(importWithoutAlias |> isParentImport) && hasAlias)) were making things too convoluted. Feel free to improve/refactor as you wish.

And lastly, I had to create a importType variable to be able to make the tests pass (having Unexpected parent import in the eslint message). I would suggest to remove this word for the sake of simplicity, but that would imply updating the tests and I didn't want to take this decision myself.

@simplenotezy
Copy link

simplenotezy commented Aug 6, 2024

@dword-design Hey Sebastian. Could we have this merged? <3

image image

@dword-design
Copy link
Owner

dword-design commented Aug 6, 2024

@simplenotezy Sry for the delay. It generally lgtm. Can we do the following changes:

  • Use 2 separate calls instead of the array destructuring? Or does the destructuring have a specific purpose?
  • Refactor the import type into a function so that we don't have the let var
  • Add a doc to the readme

Then I'm up to merge 🙂.

@chapa chapa force-pushed the siblings-and-subpaths branch from ba58977 to 49584c9 Compare August 9, 2024 11:25
@chapa
Copy link
Author

chapa commented Aug 9, 2024

Hi, no problem for the delay.

I did the requested changes but then I tested on a real project and realized a bug and a regression introduced by my modifications, those are fixed now.

After that, I realized that the two new options forSiblings and forSubpaths were not sufficient for what I wanted to do. Indeed, they didn't make some distinctions that I wanted to have, which are :

  • forcing alias for siblings only if the current file (in which the import is written) is located at the root of the alias,
  • forcing alias for subpaths only if the current file is located outside of the alias.

That's why I improved those two options to also accept objects instead of booleans. So now, forSiblings accepts an object in the shape of { ofMaxNestingLevel: number } to limit the maximum nesting level for which the alias will be enforced, and forSubpaths accepts an object in the shape of { fromInside: boolean, fromOuside: boolean } to be able to enforce aliases only for subpaths from inside or from outside the alias.

I hope those changes are not too much, I think it's ok since the boolean way sill work and the fine-grained configuration is opt-in.

Didn't have the time to update the readme yet but I'll do it (with good examples so everything is well understandable) 😉

@chapa
Copy link
Author

chapa commented Aug 10, 2024

@dword-design docs are here, feel free to merge if you think it's ok.

Just one last little thing: in my project I have some parent imports inside of an alias, which I would prefer to have in the relative path format instead of the aliased format. I'm wondering if we could add a forParents option, similar to forSubpaths which have the fromInside and fromOutside sub-options. This option would defaults to true (= enforcing aliases for all parent imports, as it is now), but setting it to { fromOutside: true } would only enforce aliases when the import is done from the outside (and enforce relative paths when it's done from the inside).

What do you think ? It could be the object of another MR though.

@dword-design
Copy link
Owner

dword-design commented Aug 12, 2024

@chapa Ok I have some time now to work on this. My basic question generally is what the use cases are of having these fine-granular options.

The idea of the plugin is to have aliases instead of having these ../../src/utils/foo.js imports. That's also why the sibling and subpath imports are enforced by default because the alias import would be longer. However, I get the use case to have aliases for everything, but this would basically be a single forSubpaths: true, nothing else 😋. Can you maybe elaborate on your use cases in general, why you need the options?

Apart from that, thanks already for the effort you put into the PR 🙂.

@chapa
Copy link
Author

chapa commented Aug 13, 2024

Sure, in my project I have some root folders for general (non-specific) stuff and I want their imports to be written with the alias everywhere in the code, but I still want relative path for imports that are "local to that stuff".

Here is a simplified example to show what's problematic with the actual implementation, I have these files:

./
├── components/                 <-- Aliased as `_/components`
│   └── MyComponent/
│       ├── components/
│       │   └── SubComponent.tsx
│       ├── MyComponent.tsx
│       └── index.ts
├── hooks/                      <-- Aliased as `_/hooks`
│   ├── useHookOne.ts
│   └── useHookTwo.ts
├── pages/
│   ├── PageOne.tsx
│   └── PageTwo.tsx
└── App.tsx

In the actual implementation:

  • if I want to import components/* or hooks/* from App.tsx it would enfore relative path (subpath) and I would prefer aliases,
    • but I still want to enforce relative paths when importing components/MyComponent/components/SubComponent from components/MyComponent/MyComponent.tsx (also subpath, but "from inside"), hence the fromInside and fromOutside sub-options for forSubpaths to differentiate those two cases,
  • if I want to import useHookOne from useHookTwo it would enforce relative path (sibling) and I would prefer aliases,
    • but I still want to enforce relative paths when importing components/MyComponent/MyComponent from components/MyComponent/index.ts (also sibling, but with a deeper nesting level), hence the ofMaxNestingLevel sub-option for forSiblings to differentiate those two cases*.

Does it make more sense ?

*: actually I first created a forRootSiblings option that answered my use case, but then I though it was too specific and conflicting with forSiblings, so I came up with the idea of a ofMaxNestingLevel sub-option.

@dword-design
Copy link
Owner

dword-design commented Aug 13, 2024

@chapa If I got it right, these components and hooks folders are some kind of "contexts" that want to be treated in isolation and if the import goes across the boundaries, it should be an alias import.

So if there is an import from App.tsx into components, it's an alias import because it goes from outside components inside it. But if I import something inside components from a file which is already in there, it's a relative import (if it's a sibling or subpath).

I don't understand why you want an alias when importing between the two hooks but not an alias if you import between components. What's the reasoning behind it?

Or, do you always want to import top-level files from components and hooks as aliases so it's clear from the import that they are "components" and "hooks"?

@chapa
Copy link
Author

chapa commented Aug 13, 2024

Yes you're right with the last sentence, I always want to import top-level files from components and hooks as aliases so it's clear from the import that they are "components" and "hooks".

I didn't give the example of importing between two (top-level) components because they all have their proper folder (unlike hooks that are all together in the hooks folder), so imports are "parent" ones so already enforced as aliases.

As I re-read you're answer, I'm thinking maybe you misunderstood the example with components/MyComponent/MyComponent.tsx importing components/MyComponent/components/SubComponent and why I want a relative path here.
I don't know if you're familiar with React, but if not you just have to know that when a component becomes too large you can extract some parts of it into other sub-components (like you would refactor a large function into multiple smaller ones), for clarity and maintainability).
Everything could very well be in the same file (e.g. components/MyComponent/MyComponent.tsx), but sometimes there is just too much to put in a single file so it's better to put components in their own files. For this kind of import (so, non top-level components), I prefer relative paths.

Is that better ? :)

@dword-design
Copy link
Owner

@chapa Ok yeah, so summing that up again:

  • There are multiple aliases that are subpaths of the root path of the project. So e.g. components, hooks, utils etc., similar to the directory structure of Nuxt.js (I think Next.js is similar, I'm coming from the Vue world).
  • Every of these aliased paths defines some stuff that is exposed to the outside and can be used elsewhere. This exposed stuff should always be imported via an alias.
  • There is also encapsulated stuff inside the folders that is consumed by the exposed stuff, but isn't exposed itself, to distribute logic. This should always be imported using relative import (also if there is a superpath in it).

The issue now is: What is exposed and how is it defined what is exposed? The obvious answer is to take the direct subfiles of the aliased paths (components/MyComponent.tsx), but it's more complicated:

  • components/MyComponent/index.tsx, so a folder per component
  • components/b/Button.tsx (having it in some subfolder and this subfolder is then prefixing the component (see e.g. Nuxt components folder)

So, the nesting level inside the aliased path doesn't necessarily say if it should be exposed. If I got it right, you tried to define that logic using the options you proposed (max nesting level, from inside, from outside, etc.).

Currently the proposal still feels a bit not thought till the end and it feels to me like a very clear use case that "should" only have a single boolean option to change the plugin to this use case.

Another idea I had was maybe to have a function/regex/matcher to decide if something should be definitely aliased. This doesn't include then where the import comes from (but maybe also doable with some from: to array), but in your case it looks a bit like you always want to use relative paths except for these specific top-level files.

@dword-design
Copy link
Owner

fyi i shipped the option to always use aliases, also for subpaths.

@chapa
Copy link
Author

chapa commented Aug 29, 2024

Hi, sorry for the delay.

Yeah you summed it up well 👍

Actually I didn't know that the .eslintrc.json (and equivalents) was deprecated in favor of eslint.config.js (and equivalents), so having a function in the plugin options didn't came to my mind, but I think it would be perfect because it's both simple, clear and powerful.

For example we could have a shouldAlias(params): boolean function with the following params:

  • source: string: path of the importing file,
  • sourceAlias: string | undefined: name of the detected alias for the importing file (if any),
  • sourceAliasPath: string | undefined: path of the detected alias for the importing file (if any),
  • destination: string: path of the imported file,
  • destinationAlias: string: name of the detected alias for the imported file,
  • destinationAliasPath: string: path of the detected alias for the imported file.

That would give all the possible flexibility to the user to decide whether the import should be aliased or not, and should answer every use cases. What do you think ?

@dword-design
Copy link
Owner

@chapa Sounds great! Feel free to provide a PR!

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.

Doesn't work for imports in the same directory
3 participants