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

feat: resolve cssPath with resolvePath #465

Merged
merged 2 commits into from May 20, 2022

Conversation

dcrall
Copy link
Contributor

@dcrall dcrall commented Apr 29, 2022

This change updates the way the cssPath option is resolved. Instead of resolveAlias it uses resolvePath This is the same mechanism used for the resolution of configPath. This allows module users to resolve a stylesheet provided by a dependency. In our case, we have a component library that includes a tailwind.css file that we would like to share between applications.

Testing change

You can see annotated results in the screenshot below:

only-tailwind-base-loaded

Allow consumers to resolve a stylesheet provided by a dependency.
@Atinux
Copy link
Collaborator

Atinux commented May 2, 2022

I am not sure to understand the issue here, do you have an example of the error created by resolveAlias?

By not using resolveAlias, the end user won't be able to use the ~ to link to its css file (ex: ~/assets/my-tailwind.css).

@dcrall
Copy link
Contributor Author

dcrall commented May 10, 2022

@Atinux Thank you for reviewing this change. Sorry for my slow response. I was out of the office last week.

resolvePath calls resolveAlias, so the common case of ~/assets/tailwind.css works as expected. Here is the call to resolveAlias.

To illustrate this, I've created three branches and pull requests under my fork that use the package @dcrall/tw-test, which simply exposes a tailwind config and stylesheet. In each branch, I've included screenshots showing the behavior.

The first branch shows the current module with the addition of a logging statement. No errors are thrown, but the module loads its own runtime/tailwind.css instead of the specified file from the package.

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 ~/assets/tailwind.css.

Please let me know if you have any more questions or concerns.

Copy link
Contributor

@pi0 pi0 left a 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.

@pi0
Copy link
Contributor

pi0 commented May 12, 2022

@dcrall Can we maybe improve this by providing extensions option? By default Nuxt resolves with all possible extensions such as .js and .vue while we probably only want to allow .css (and other style related ones)

@pi0 pi0 changed the title Resolve cssPath with resolvePath feat: resolve cssPath with resolvePath May 12, 2022
@dcrall
Copy link
Contributor Author

dcrall commented May 16, 2022

@pi0 I understood your extension comment to be a request to limit file resolution to .css (and related) files. Looking at resolvePath that does not seem to be the purpose of the extensions option. My testing indicates that will resolve ~/assets/tailwind as ~/assets/tailwind.css or ~/assets/tailwind as ~/assets/tailwind/index.css.

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?

@Atinux
Copy link
Collaborator

Atinux commented May 18, 2022

From what I see here it works if you specify the extension in cssPath which anyway seems the best when dealing with non js-file and npm packages.

Thank you so much for all your reproductions and tests, I believe it is good to merge in that state 😊

@pi0
Copy link
Contributor

pi0 commented May 20, 2022

It works for resolving files in the project, but the resolution fails when resolving to a file in a package.

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 exports subpaths.

Copy link
Contributor

@pi0 pi0 left a 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')
Copy link
Contributor

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.

@pi0 pi0 merged commit 656eea5 into nuxt-modules:main May 20, 2022
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.

None yet

3 participants