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

Bug with multiple @apply in one parent #8203

Closed
psucodervn opened this issue Apr 26, 2022 · 7 comments · Fixed by #8213
Closed

Bug with multiple @apply in one parent #8203

psucodervn opened this issue Apr 26, 2022 · 7 comments · Fixed by #8213

Comments

@psucodervn
Copy link
Contributor

Consider the line 312 here

let candidates = perParentApplies.get(apply.parent) || []
perParentApplies.set(apply.parent, [candidates, apply.source])

Should it be

let candidates = (perParentApplies.get(apply.parent) || [[]])[0];

instead?

@thecrypticace
Copy link
Contributor

It should be let candidates = [] since we've just initialized perParentApplies and are then immediately overwriting that key in perParentApplies. I pushed the change for this to master. The existing code isn't a problem — it's just unnecessary. Thanks for pointing it out 🤦

As for multiply uses of @apply in a single rule we handle that because we split one rule into 2 or more whenever we encounter an @apply.

@psucodervn
Copy link
Contributor Author

Hi @thecrypticace , thanks for the quick response. Yeah, I know that the lib itself works well with the current code (because that rules with multiple @apply were already split, as you said). But other libraries may use this function without notice about this, and it would cause some related issues.

For example, in my case, I'm using Tailwind CSS IntelliSense extension in vscode. This extension import and use the expandApplyAtRules function (https://github.com/tailwindlabs/tailwindcss-intellisense/blob/master/packages/tailwindcss-language-server/src/server.ts#L662). Then when I'm including a plugin that have a rule that contains multiple @apply (i.e. @vechaiui/core), the extension will failed to process.

My example tailwind.config.js:

module.exports = {
  ...
  plugins: [
    require('@vechaiui/core'),
  ],
}

Your new code (let candidates = []) will let the extension continuing to execute, but it will eliminate other @applys in a rule that have multiple of them.

@thecrypticace
Copy link
Contributor

Understood. In this case that would be a bug in the language server that needs to be addressed.

@thecrypticace
Copy link
Contributor

@psucodervn I just did a little test with the intellisense extension and so far am not having problems. It handles the multiple applies as expected. Can you prep a small reproduction of what you're seeing? It'd be much appreciated!

Plugin:
Screen Shot 2022-04-27 at 09 47 51

Popup from intellisense:
Screen Shot 2022-04-27 at 09 45 19

@thecrypticace thecrypticace reopened this Apr 27, 2022
@psucodervn
Copy link
Contributor Author

Hi @thecrypticace, thanks for the investigation. Here is a minimal project repo to reproduce the issue: https://github.com/psucodervn/tailwind-intellisense-issue.

Hope it helps.

@thecrypticace
Copy link
Contributor

I've pushed a fix and it should be in our insiders build in a few minutes. I used something fairly close to what you wrote so I credited you as a co-author. Thanks again for the persistence and I'm so sorry for all the confusion!

@psucodervn
Copy link
Contributor Author

Thanks for the quick and clean fix @thecrypticace 😄

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 a pull request may close this issue.

2 participants