-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
PR [Feature]: Use Tailwind's built-in Plugin API and configure Intellisense. #93
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for monaco-tailwindcss ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
This PR does too many things. Please keep it focused. I don’t see the need to for the changes to the plugin API and unrelated changes. Let’s keep focus on supporting the configuration API.
"directories": { | ||
"example": "examples" | ||
}, | ||
"types": "./index.d.ts" | ||
} |
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.
What’s with all these metadata changes? None of the changes here are needed. I also noticed you published your fork as monaco-tailwind
and didn’t update any of the metadata? Forking is fine, publishing too, but using such a similar name to publish a fork doesn’t seem like the nices thing to do. It’s also misleading to both your and my users to point your metadata at my project.
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.
Yeah, as I said, this is my very first time contributing, doing PR. I've published a package to test it on the project where I use Monaco. I only later discovered that I could have tested it directly with the demo in the repo. My bad on that. As I knew I would eventually delete the package, I didn't bother to update the metadata accordingly. So no worries about that, I'll update it and delete the package if the changes get merged here.
@@ -10,7 +10,7 @@ export interface TailwindWorkerOptions { | |||
* @returns | |||
* A valid Tailwind configuration. | |||
*/ | |||
prepareTailwindConfig?: (tailwindConfig?: TailwindConfig | string) => Config | PromiseLike<Config> | |||
prepareTailwindConfig?: (tailwindConfig?: TailwindConfig | string) => Config |
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.
Why was this changed?
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 did that in early experiment, trying to get things working. I've put it back as original afterwards, I'll fix it in next commit.
} | ||
} | ||
*/ | ||
intellisense?: Partial<{ |
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.
VSCode calls this settings. LSP calls it configuration. Since we already have so many things called configuration
, I’m leaning towards calling this property settings
.
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.
Sounds good
recommendedVariantOrder: DiagnosticSeveritySetting | ||
} | ||
}> | ||
}> |
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 think all of these nested properties should be optional, also the deeply nested ones. Since we’re defining the object, use the optional property symbol ?
, not the Partial
type.
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.
Noted
showPixelEquivalents: boolean | ||
rootFontSize: number | ||
colorDecorators: boolean | ||
files: { exclude: string[] } |
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 think this property is meaningless to the Monaco implementation? Let’s omit it. Also consider for each property what it does, and whether it makes sense to define it.
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 frankly don't know which properties are supported in Monaco or even what they do. I just guessed that as they're present (even if empty) in the default settings used, I might as well give access to them. Maybe should we accept only the one that probably matters the most, classAttributes
?
// Default values are based on | ||
// https://github.com/tailwindlabs/tailwindcss-intellisense/blob/v0.9.1/packages/tailwindcss-language-server/src/server.ts#L259-L287 | ||
tailwindCSS: { | ||
emmetCompletions: false, | ||
classAttributes: ['class', 'className', 'ngClass'], | ||
classAttributes: ['class', 'className', 'ngClass', ...classAttributes], |
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.
Should the user provided class attributes extend the defaults? Or should it replace them? What does the language server do?
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 know that in VS Code you can directly edit the list of attributes within the settings panel or json. Here, we have to go another route. We either extend or override. In my opinion, we should extend with at least class
and className
as default. You wouldn't want to pass ['styles']
and lose completion/hover on class
and className
. I think the expected behavior is that styles
gets added to the existing list.
If we really want to leave the choice to the user:
// classAttributes would look like this
type classAttributes = { extend: string[] } | { override: string[] }
// and the implementation like this
tailwindCSS: {
classAttributes: 'extend' in classAttributes
? ['class', 'className', ...classAttributes.extend]
: classAttributes.override
}
* } | ||
|
||
// ...you provide an array containing the arguments | ||
// for each function you wish to call. |
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.
This is your interpretation of how the configuration could work. This might cater to your specific situation, but it might not to others’. The prepareTailwindConfig
option from initialize
allows you to fully customize how the Tailwind config provided could be interpreted.
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.
Alright, this whole thing was a big overlook, might call it a skill issue, on my side... Remember in the issue that led to this PR when I said I tried initialize
but didn't make it work and couldn't really understand what is its purpose?
I was calling that thing just above window.MonacoEnvironment
where I create the different workers... This is also my first time using workers, I didn't know better. Now, I've created a file for the worker, use initialize
in it and point the tailwindcss
worker to that file. And it just works...
Sorry for this whole debacle and wasted time... Also for the package I've published. It was never my intention to mislead anyone. I'm going to delete it right now. Anyway, I think that the |
This PR enhances the capabilities of
configureMonacoTailwindcss
. You can now extend the settings of Intellisense, like adding someclassAttributes
that will triggerautocompletion
. And most importantly, you have access to Tailwind's built-in Plugin API. Instead of calling them using theplugins
property of your config, you provide the arguments of each function you wish to call within apluginAPI
property.