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

Bump less-cache to 2.0.1 #644

Merged

Conversation

savetheclocktower
Copy link
Contributor

Identify the Bug

Fixes the bug that was addressed in less-cache#6.

Description of the Change

Inline JavaScript (within backticks) will once again be tolerated within certain LESS constructs. This is a feature I was using so that I could write

.s('string.quoted.double', { color: green; });

instead of

.syntax--string.syntax--quoted.syntax--double {
  color: green;
}

Less.js has deprecated this behavior, but we haven’t, and it’s good not to break stuff.

Alternate Designs

There is an alternative way of pulling this off with @plugin rules, but it only works in Less v4, and there’s no simple way to offer up the right solution for the user’s Pulsar version, so I’ll hold off on adopting that approach in my syntax theme until I’m certain it won’t break for the entirely fictional other people who are also using vibrant-ink-redux as their syntax theme.

Possible Drawbacks

I can’t think of any. This is no less secure than what we were already doing, and I don’t think it’s a security problem at all, since the JS inside a less file is ultimately no less sandboxed than the JS we allow a package to run otherwise.

Verification Process

Wrote a unit test in less-cache to demonstrate the fix. If CI passes, we should be good.

Release Notes

Restored ability for less files in packages to use inline JavaScript inside backticks

Fixes inline JavaScript breakage in `less` files.
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

LGTM! Simple enough change, bumped exactly what needs to be. Only thing better is getting the yarn lock file synced so Renovate doesn't freak out, but really not an issue.

Approving since the only CI failure (ARM Linux) is failing yet again with strange SSL issues, which we know is due to CI issues. So hoping we can get those successful after a run or two lets get this merged.

@confused-Techie
Copy link
Member

Thanks for adding that last bit I mentioned! After a few reruns I'm still not getting ARM to be happy, but the failure looks to be totally caused by CI issues. So lets go ahead and merge this

@confused-Techie confused-Techie merged commit b731764 into pulsar-edit:master Jul 19, 2023
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.

2 participants