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

Use absolute import paths (/foo) instead of relative (../../foo) ones #8190

Closed
tofumatt opened this issue Jul 25, 2018 · 5 comments
Closed
Assignees
Labels
[Status] Not Implemented Issue/PR we will (likely) not implement. [Type] Code Quality Issues or PRs that relate to code quality

Comments

@tofumatt
Copy link
Member

This started in https://wordpress.slack.com/archives/C02QB2JS7/p1531255855000083

We use relative import paths (../../foo) all over the place, but I think it makes it really difficult to reason about where the code is in the project and it makes it harder to open that file because you can't just copy the path and open it in your editor. The only real benefit is that it makes imports within a module self-contained, but as refactors happen less than wanting to open a related file, I think that's a weak argument for keeping relative imports. I find the cognitive overhead of stepping through a number of ../s to be high as well. Take, for instance, something like: https://github.com/WordPress/gutenberg/pull/8188/files#diff-4651e808991ae4dc4bd68adfb417d888R11

I think we should be using absolute import paths everywhere.

@tofumatt tofumatt added the [Type] Code Quality Issues or PRs that relate to code quality label Jul 25, 2018
@tofumatt tofumatt self-assigned this Jul 25, 2018
@sarahmonster
Copy link
Member

As a not-developer, I would like to +100 this. It's especially confusing when sometimes there are multiple directories with the same name (components, for example.)

@markjaquith
Copy link
Member

And a more extreme example: ../../../../../ (imports from a Jest test). I struggle to even count how many upward traversals that is.

@designsimply designsimply added the [Type] Question Questions about the design or development of the editor. label Jul 25, 2018
@nerrad
Copy link
Contributor

nerrad commented May 27, 2019

This issue has been open for a while, what next steps are needed to move it forward? Is it still an issue needing resolved?

On the surface, here's some thoughts on what would need to happen:

  • bring it up in a #core-js or #core-editor meeting to decide if this is something the project wants to adopt. If the decision is "yes"...
  • identify what contributor docs need updated.
  • identify if there are any es-lint rules needing updated.
  • Would webpack config need updated to set what is the "root" for imports?
  • Would Jest config need updated - likely this would be an override for the GB project only or would configs wp-scripts come out of the box with absolute import support?

Some considerations:

  • There are a LOT more packages since this issue was created. How feasible will it be to modify all the files to use absolute imports? Is the time investment worth it?
  • GB has a complicated build process, how will absolute imports be resolved in the webpack config? Is this impacted by the Lerna mono repo build process at all?

Absolute imports are attractive to me to get rid of the ../ hell and guesswork. However I'm not sure its worth the effort now (especially when it's been nearly a year since this issue opened without much action).

@gziolo
Copy link
Member

gziolo commented May 28, 2019

And a more extreme example: ../../../../../ (imports from a Jest test). I struggle to even count how many upward traversals that is.

This is no longer an issue since Enzyme was upgraded to support React 16.3.

There are still 2 places when files are imported by traversing to the top of repository and down:

const focus = require.requireActual( '../../../../dom/src' ).focus;

export * from '../../../../packages/block-library/src';

Otherwise, all imports are within the package. I would personally check whether we can improve something for those 2 occurrences and close this issue.

@gziolo gziolo added Needs Decision Needs a decision to be actionable or relevant and removed [Type] Question Questions about the design or development of the editor. labels May 28, 2019
@aduth
Copy link
Member

aduth commented Jul 24, 2019

I would personally check whether we can improve something for those 2 occurrences and close this issue.

I agree. The problem in these cases is that we should be traversing outside a package's own files. Within the context of a package, there should not be much folder depth. Arguably, the relative traversal can serve as a deterrent in order to encourage a flatter folder structure.

@aduth aduth closed this as completed Jul 24, 2019
@aduth aduth added [Status] Not Implemented Issue/PR we will (likely) not implement. and removed Needs Decision Needs a decision to be actionable or relevant labels Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Not Implemented Issue/PR we will (likely) not implement. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

7 participants