-
Notifications
You must be signed in to change notification settings - Fork 184
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: resolve cssPath
with resolvePath
#465
Conversation
Allow consumers to resolve a stylesheet provided by a dependency.
I am not sure to understand the issue here, do you have an example of the error created by By not using |
@atinux Thank you for reviewing this change. Sorry for my slow response. I was out of the office last week.
To illustrate this, I've created three branches and pull requests under my fork that use the package The first branch shows the current module with the addition of a logging statement. No errors are thrown, but the module loads its own The second branch shows the modified module successfully resolving the package's tailwind.css. The third branch shows the modified module successfully resolving the stylesheet from Please let me know if you have any more questions or concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. resolvePath
internally handles aliases too.
@dcrall Can we maybe improve this by providing extensions option? By default Nuxt resolves with all possible extensions such as |
cssPath
with resolvePath
@pi0 I understood your extension comment to be a request to limit file resolution to I have illustrated that case in the PR below. It works for resolving files in the project, but the resolution fails when resolving to a file in a package. To make the package resolution work, we would need to try module resolution against the extension array similar to the code above it. Have I misunderstood? Are you thinking I should check the configured path against the extensions array? |
From what I see here it works if you specify the extension in Thank you so much for all your reproductions and tests, I believe it is good to merge in that state 😊 |
This is almost correct. Using extensions is mostly useful for relative fs paths and not package resolution. Referencing to packages is probably even more beautiful without any extension and using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -49,6 +48,7 @@ export default defineNuxtModule({ | |||
logger.info(`Using Tailwind CSS from ~/${relative(nuxt.options.srcDir, cssPath)}`) | |||
nuxt.options.css.splice(injectPosition, 0, cssPath) | |||
} else { | |||
logger.info('Using default Tailwind CSS file from runtime/tailwind.css') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this isn't too verbose to be an info log (could be debug). Almost any internal of Nuxt is also adding kinda defaults.
This change updates the way the
cssPath
option is resolved. Instead ofresolveAlias
it usesresolvePath
This is the same mechanism used for the resolution ofconfigPath
. This allows module users to resolve a stylesheet provided by a dependency. In our case, we have a component library that includes atailwind.css
file that we would like to share between applications.Testing change
runtime/tailwind.css
.You can see annotated results in the screenshot below: