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

Exclude test files from glob pattern #662

Merged
merged 4 commits into from
Apr 27, 2023
Merged

Conversation

maartenvanhunsel
Copy link
Contributor

@nuxt-studio
Copy link

nuxt-studio bot commented Apr 20, 2023

Live Preview ready!

Name Edit Preview Latest Commit
TailwindCSS Edit on Studio ↗︎ View Live Preview 0cb2bdf

@ineshbose
Copy link
Collaborator

This is a very good issue - thanks for pointing it out.

I feel this change may make the provided options more complex for developers and it gets used in their configuration. The module does provide an option to provide content as a function to override it, but that's still some work required from developers.

Do you think that maybe adding a separate glob string to the list may be useful? Something like:

`!${srcDir}/**/*.{spec,test}.*` // mind the ! at the start

@Atinux
Copy link
Collaborator

Atinux commented Apr 26, 2023

I prefer your solution @ineshbose

Happy to update your PR @maartenvanhunsel ?

@maartenvanhunsel
Copy link
Contributor Author

@ineshbose @Atinux sorry for late response, just came back from a weekend trip. Yea sure, the solution you @ineshbose provided is way more clean and readable.

@Atinux just updated the docs.

@Atinux Atinux merged commit ee83cea into nuxt-modules:main Apr 27, 2023
4 checks passed
@Atinux
Copy link
Collaborator

Atinux commented Apr 27, 2023

Will make a release once we fix the tests

@maartenvanhunsel
Copy link
Contributor Author

@Atinux @ineshbose just had time to test if the error still occurred and unfortunately it is.
Seems ${srcDir}/components/**/*.{vue,js,ts} is causing the error. ${srcDir}/components/**/!(*.{test,spec}).{vue,js,ts} is solving this.

See reproduction with updated Nuxt tailwind version: https://stackblitz.com/edit/github-huekcx-ch2uja?file=nuxt.config.ts

Any thoughts?

@ineshbose
Copy link
Collaborator

@maartenvanhunsel thanks for reporting back; I did have minor hesitations with that, and your new glob seems to be working better. So if we generalise it to other directories and extensions, would this work? (Stackblitz is acting weird for me to test changes)

  `${srcDir}/**/!(*.{test,spec}).*`

Please feel free to open a new PR btw!

@maartenvanhunsel
Copy link
Contributor Author

maartenvanhunsel commented Apr 29, 2023

@ineshbose I've already tested a lot of glob patterns but couldn't make it work with the exclude glob pattern. ${srcDir}/components/**/*.{vue,js,ts} is still causing the error.

@ineshbose
Copy link
Collaborator

Oh, you meant that the glob is a modification to the first string rather than appending. Let's investigate further then!

@ineshbose
Copy link
Collaborator

You're very right.

I'm unable to think of how to solve this without modifying the existing content paths - but then they may get complicated. The added glob is not really helping (and in my case seems to be increasing the load for JIT).

Moreover, there are also more cases like *.{stories,story}.* that may need to be covered if we are to exclude such files from content configuration.

My thinking is to put the configuration to be done by developers in this case (since the module already provides content overriding option) and modify the documentation for it with an example for this; because there may be many ways that developers organise their test files (like if it were in a test/ directory, this wouldn't be an issue).

export default {
  content (contentDefaults) {
    return [
       ...contentDefaults,
       // contentDefaults.map(c => c.endsWith('/**/*.{vue,js,ts}') ? c..replace('/**/*.{vue,js,ts}', '/**/!(*.{test,spec}).{vue,js,ts}') : c),
       contentDefaults[0].replace('/**/*.{vue,js,ts}', '/**/!(*.{test,spec}).{vue,js,ts}'),
    ]
  }
}

What do you say @maartenvanhunsel ? While I'd love the module to consider ignoring tests out the box, it may be counter-intuitive for maintainability (although Atinux is the one who will call the shots 😄).

Please do share if an ideal solution comes to mind.

@maartenvanhunsel
Copy link
Contributor Author

@ineshbose that's exactly my workaround I'm already using haha ;)

This workaround is perfect to me, however it will take time to investigate for new people who encounter this errors because the logs are not very clear.

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