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

Update module-path-aliases.md #43592

Merged
merged 5 commits into from Jun 14, 2023
Merged

Update module-path-aliases.md #43592

merged 5 commits into from Jun 14, 2023

Conversation

wiredacorn
Copy link
Contributor

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have a helpful link attached, see contributing.md

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • e2e tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have a helpful link attached, see contributing.md

Documentation / Examples

  • [X ] Make sure the linting passes by running pnpm build && pnpm lint
  • [X ] The "examples guidelines" are followed from our contributing doc

@@ -93,3 +93,35 @@ export default function HomePage() {
)
}
```

The additional `paths` are reletive to the `baseUrl`. For example:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The additional `paths` are reletive to the `baseUrl`. For example:
The additional `paths` are relative to the `baseUrl`. For example:

"paths": {
"@styles/*": ["../styles/*"],
"~/*": ["../*"],
"/*": ["../../*"]
Copy link
Member

Choose a reason for hiding this comment

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

I think this example may cause confusion as baseUrl shouldn't really be used in this way and more so should point to the packages base. Potentially it could show a nested package as the baseUrl and including shared utils in paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ohk. I am pretty new to NextJS, so I was not aware this is a bad practice. It does seem to be working in my project, but I think we could just remove this example.

I do think that it would still be helpful to mention that the paths will start at the baseUrl location instead of the root.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sounds good to keep just the note and maybe strip the example 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have removed the example and also added a commit with a better example in case you want to include one.

@ijjk ijjk force-pushed the canary branch 2 times, most recently from e078ebe to 6b863fe Compare December 2, 2022 05:49
@@ -93,3 +93,35 @@ export default function HomePage() {
)
}
```

Each of the `"paths"` are reletive to the `baseUrl` location. For example:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Each of the `"paths"` are reletive to the `baseUrl` location. For example:
Each of the `"paths"` are relative to the `baseUrl` location. For example:

@ijjk ijjk requested a review from jankaifer as a code owner March 2, 2023 23:39
@ijjk ijjk requested a review from a team as a code owner May 4, 2023 16:19
# Conflicts:
#	docs/advanced-features/module-path-aliases.md
@ijjk ijjk merged commit cc46d97 into vercel:canary Jun 14, 2023
31 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants