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: add css-in-js tag support #26

Merged
merged 39 commits into from
Feb 5, 2023
Merged

feat: add css-in-js tag support #26

merged 39 commits into from
Feb 5, 2023

Conversation

Ilanaya
Copy link
Owner

@Ilanaya Ilanaya commented Jan 17, 2023

Resolves #25

@Ilanaya Ilanaya marked this pull request as draft January 17, 2023 08:55
@Ilanaya Ilanaya changed the title feat: add javascript support feat: add langs with tagged template syntax support Jan 18, 2023
@Ilanaya
Copy link
Owner Author

Ilanaya commented Jan 18, 2023

@zardoy Can you check, please? Didn't get how to determine if the completions request was invoked in tagged template, so I decided to pass full document text for now. Maybe you can give me a hint?

@zardoy
Copy link
Collaborator

zardoy commented Jan 19, 2023

Since this change would make new release also would be good to update tailwind deps to latest

@zardoy
Copy link
Collaborator

zardoy commented Jan 19, 2023

@maIIady short answer: you can try to adopt typescript plugin from https://github.com/zardoy/vscode-better-snippets to check the kind of current node and then return its start & end. If you don't have much time or struggling with adopting TS plugin code you can use alternative way: TypeScript Essential Plugins API e.g.: https://github.com/zardoy/vscode-experiments/blob/bec7c7f70d7b15cd323a0c1097496e5112fc7ef7/src/features/tsPluginIntegrations.ts#L17 (check kind + styled.* before start), but it'd require this plugin to be installed & active which is not ideal, but better than nothing it might be okay for this time

@Ilanaya
Copy link
Owner Author

Ilanaya commented Jan 19, 2023

Ok, thanks. I think I'll go with the second suggested solution for now and rewrite to use adopted TS plugin later. Hope @alvarosoaress wouldn't mind installing an excellent extension.

@zardoy
Copy link
Collaborator

zardoy commented Jan 19, 2023

@maIIady Have you a chance to try it to use with volar?

ps You can also probably build TS-checking or any other ls features

@Ilanaya
Copy link
Owner Author

Ilanaya commented Jan 19, 2023

@maIIady Have you a chance to try it to use with volar?

ps You can also probably build TS-checking or any other ls features

If I'll fugure out cases and how to use it there, of course there is a chance :)

@Ilanaya
Copy link
Owner Author

Ilanaya commented Jan 20, 2023

Made it work, gonna add tests and ts extension requirement soon

Copy link
Collaborator

@zardoy zardoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This abbreviation doesnt work properly:

css`
px-1
`

This is because the default range of completions is derived from word pattern of current language. - is included into regex of word pattern in css (source) but not in JS/TS (source).

Just patch range of returning completions to document.getWordRangeAtPosition(position, /[-\w\d]+/)

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
Ilanaya and others added 2 commits January 21, 2023 15:53
@zardoy
Copy link
Collaborator

zardoy commented Jan 21, 2023

@maIIady Your refactor is exactly what I wanted to see 👏. Lastly, to support LastTemplateToken try to use tsEssentialPlugins.getNodePath command instead that will return array of { kindName, start, end } (where last item is what we're using now)

Copy link
Collaborator

@zardoy zardoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I didn't notice it earlier but it seems we publish extension with registerActiveDevelopmentCommand which is bad as it's being registered on production, can it be removed from sources?

src/shared.ts Outdated Show resolved Hide resolved
@Ilanaya
Copy link
Owner Author

Ilanaya commented Jan 25, 2023

Made it work in all parts of the tagged template (suggestions work like a charm in my tests). However, there are cases when usedRules strikethrough/deletion doesn't work correctly which I tried to fix here too.
Given this CSS:

.test {
  padding-left: 1px;
  padding-right: 1px;
  display: flex;
  flex|(trigger suggestions) - works correctly
}
.test {
  padding-left: 1px;
  padding-right: 1px;
  flex|(trigger suggestions) - doesn't strikethrough already used rule
  display: flex;
}

A weird postCssParser error line causes this: e.g. for the second case calling error.showSourceCode() result in

   3 |   padding-right: 1px;
   4 |   flex
 > 5 |   display: flex;
     |   ^
   6 | } 

This makes sense but dunno how to handle this in extension code correctly.

Gonna think about it a little bit later when writing tests, currently just have no ideas. Maybe just keep this behaviour for now?

src/parseCss.ts Outdated Show resolved Hide resolved
const pos = str.indexOf('|')
const strToParse = str.slice(0, pos) + str.slice(pos + 1)

const { usedRules } = parseCss(strToParse, pos)
Copy link
Collaborator

@zardoy zardoy Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maIIady I finally managed to review normalizeStylesContent usage, because I was sure we should call parseCss only twice if needed. Unfortunately its a completely wrong way to do things here, for example here you're passing TS code to parseCss code that accepts, you obviously should not try to fix and pass valid css when it makes sense.

Most common case I can imagine we should support:

css`
    display: flex;
    ${'...'/* absolutely don't care */}
    color: red;
    ${'...'/* absolutely don't care */}
    flex|
    ${'...'/* absolutely don't care */}
`

In most cases dynamic variable or conditional logic is used within these blocks.

Ideally we extract css to parse:

display: flex;
color: red;
flex|

But since there is no api we have two choices:

  • build that api (should be not that hard as we know pattern, but we also need to handle map offsets, I think i will do it some time later)
  • use range for virtual document of last node instead (will be good for first time)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zardoy To clarify, do you want me to roll back the implementation of the src/getTaggedTemplateLangsStylesRange.ts to 71329a0?

Btw I can't test if it is working right now. It falls with this error and lowering the version of the essential plugins doesn't help (I tried 0.0.42 instead of the latest). Is it the same for you?
image

Copy link
Collaborator

@zardoy zardoy Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maIIady Sorry, didn't see your question earlier.

Btw I can't test if it is working right now.

Isn't that because you're starting in mode with all extensions disabled? You need F5 when no development window is opened. Everything works on my side.

@zardoy To clarify, do you want me to roll back the implementation of the src/getTaggedTemplateLangsStylesRange.ts to 71329a0?

Not really, I say that for this code:

css`
    ${'...'}
    display: block;
    flex|
`

parseCss for now receives whole content css tagged template of including js (${'...'}) which is incorrect for js and thats why you handle patching of incorrect css so many times to get rid of css errors.

Instead, here

https://github.com/maIIady/vscode-tailwind-in-css/blob/3e5287ee1d3323e6d60c3e39f5c9242c762a727e/src/getTaggedTemplateLangsStylesRange.ts#L26

pass range of last node instead which will be valid css string and no need to handle fixing input so many times

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maIIady You did resolve this, but invalid tests are still here.. Please see start of this thread and code location

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't find a quick way to implement getTaggedTemplateStylesRange testing. Should I use https://www.npmjs.com/package/@vscode/test-electron or is there a simpler way of mocking vscode API? (actually, I struggled to pass document to the getTaggedTemplateStylesRange)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add right now (adopt from vscode-fix-all-json)

@zardoy zardoy changed the title feat: add langs with tagged template syntax support feat: add css-in-js tag support Feb 1, 2023
@zardoy
Copy link
Collaborator

zardoy commented Feb 1, 2023

@maIIady eta? I think i can finish it

@Ilanaya
Copy link
Owner Author

Ilanaya commented Feb 1, 2023

@zardoy Won't have time to work on it today. You can finish

src/getTaggedTemplateLangsStylesRange.ts Outdated Show resolved Hide resolved
@zardoy zardoy marked this pull request as ready for review February 4, 2023 22:32
@zardoy
Copy link
Collaborator

zardoy commented Feb 4, 2023

Btw I can't test if it is working right now. It falls with this error and lowering the version of the essential plugins doesn't help (I tried 0.0.42 instead of the latest). Is it the same for you?

@maIIady Randomly I've found the bug. Most probably you had vscode.typescript-language-features builtin extension disabled to activate takeover mode, this is why it was internally never activating. This is was intentional: if builtin ts ext is disabled -> then we don't have a way to pass settings from vscode to typescript plugin (since ts extension is disabled), but this is no longer true since volar takeover mode is supported, I'll fix it in this extension.

@zardoy
Copy link
Collaborator

zardoy commented Feb 5, 2023

finally!

@maIIady can you check if all is okay?

@zardoy zardoy merged commit 9eebd5d into main Feb 5, 2023
@zardoy zardoy deleted the issue-25 branch February 5, 2023 01:02
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.

Add JavaScript as a active language
2 participants