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

[core]: Bump less-cache to v2.0.0 Upgrades less to 4.1.3 #611

Merged
merged 17 commits into from
Jul 3, 2023

Conversation

confused-Techie
Copy link
Member

@confused-Techie confused-Techie commented Jun 28, 2023

This PR brings us the newest changes within less-cache:

  • Updating less from 3.12.2 -> 4.1.3
  • Normalizing all paths to use / on Linux, macOS, and Windows
  • Decaffeinating the CoffeeScript source code

This now means we are using Less v4, which does come with some breaking changes to existing Less StyleSheets. For one of these breaking changes it's possible to set a flag within Less to remain using the older functionality, but on Discord it was determined to move forward with the new code, and instead apply fixes for all breakage to existing grammars. The biggest breaking changes can be read more about on the [email protected] release. I've also added a notice to the CHANGELOG of this breakage, and urge any and all users that see this to assist in possible with fixing syntax themes in the wild.

Because of these changes, some fixes had to be made within Pulsar itself, and our core packages/themes. Some of these were expected, such as wrapping math within () while others where surprising, such as failing to built a stylesheet on undefined variables, which made me discover long abandoned and unused styles within settings-view attempting to use variables, that as far as I can tell since their inception have always been invalid. So offending blocks, where this invalid CSS was all that existed have been removed totally.

Otherwise these changes have been tested with all of our core Themes and packages, and seems no other packages need any manual changes.

Additionally, this PR will nearly help us close #468

Curbing breaking Changes

After reading the above, which is unedited since my original creation of this PR, you can see the concerns of causing breaking changes in loads of community packages, but my latest comment details how I thought we could protect against this in community packages, by inspecting the CSS style sheets before applying them, much like we already do for the scope selectors themselves.

Those changes have been completed, and will be pushed to this PR soon, after creating some tests.

But essentially, after we look for deprecated syntax selectors, we will also go ahead and search for any mathematical expressions contained within the source CSS that is not escaped properly.
This will not occur for bundled packages, much the same way we don't inspect bundled packages for deprecated selectors. Really much of the functionality choices have been taken directly from how we handle deprecated selectors. With that said, a quick proof of this functionality can be seen below:

As issue #612 shows, one of the breaking changes effected the one-dark-ui CSS for the status bar. Looking there for how this broke the status bar styling, if we again, leave our non-updated less style sheet containing the following:

.inline-block {
    margin: 0; // override default
    padding: 0 @status-bar-padding/2; // TODO fix after testing
    vertical-align: top;

    &:hover {
      text-decoration: none;
      background-color: @level-3-color-hover;
    }
    &:active {
      background-color: @level-3-color-active;
    }

    // reset on child inline-block
    .inline-block {
      margin: 0;
      padding: 0;
    }
  }

Where we have our comment to fix this, before the our newest protections on reading the compiled CSS where created, would result in the following:

image

But now, with our protections, we can see the resulting CSS is as follows:

.status-bar .inline-block {
  margin: 0;
  padding: 0 calc(1.5rem/2);
  vertical-align: top;
}

Which gives us the CSS we would expect to see here, styling the page as we would expect.

Additionally, since we had to manually restyle the CSS to work properly, we can now see this in our deprecations, where if we investigate the message via Deprecation Cop we get the following string:

[node_modules\one-dark-ui\index.less](file:///D:/pulsar-edit/pulsar/node_modules/one-dark-ui/index.less)

Starting from Pulsar v1.107.0, less v4.1.3 is used to transpile less style sheets. This means that Parens-division is now the default math setting, and all less style sheets must wrap division within parenthesis. To prevent breakage with existing style sheets, Pulsar will automatically wrap any mathematical expressions found unparsed by Less with calc(). Upgrading the values of the following properties:

* padding: 0 1.5em/2 => 0 calc(1.5em/2)

Please, make sure to upgrade usage of mathematical expressions within less style sheets.

So we can see, not only do these protections keep community packages less style sheets working in the vast majority of circumstances, it will also make users much more aware of this breakage, and hopefully let people fix these on their own time, rather than having things break immediately.

Obviously, since we are using a long complex regex to check for these mathematical expressions, there will be edge cases where we won't be able to manually inject proper styling, but I hope that this is able to resolve enough of these issues, that the vast majority of users are unaffected.


Last but not least, for ease in writing the full changelog later, here are the full updates within less-cache.

less-cache

@confused-Techie confused-Techie mentioned this pull request Jun 28, 2023
5 tasks
neil-lindquist added a commit to neil-lindquist/SLIMA that referenced this pull request Jun 30, 2023
Notably: wrapping math in parenthesis for
pulsar-edit/pulsar#611
@confused-Techie
Copy link
Member Author

I've been thinking about this change, worrying about breaking existing packages, that may not be actively maintained. And while of course I'd recommend we get as many community packages updated for this change, I believe I've found a way we could greatly minimize the impact of this to all existing packages. I'll copy and paste my message on Discord here:


So actually on the css-cache thing I think I found a sorta hacky way to keep from breaking a whole bunch of existing package's CSS.
Basically, within style-manager we already have transformDeprecatedShadowDOMSelectors() which is a function that actually inspects the content of the CSS that will be applied to the page before it's ever used, and currently it will actually modify CSS selectors, swapping out anything from deprecated-syntax-selectors.js with the more up to date form. But we could actually use that, and inspect the value of any given CSS rule, and if that rule has mangled math (which we would probably just check for / since I don't think that's valid in any rule value, it's only valid elsewhere as far as I know) then we can attempt to do the math ourselves, and return the resulting value. Sure it's not as perfect as if we had Less properly do it, but that sounds way better to me than breaking a whole load of unmaintained packages/ui themes/syntax themes

@confused-Techie confused-Techie mentioned this pull request Jul 2, 2023
5 tasks
@savetheclocktower
Copy link
Contributor

I think this will be fine as a compatibility measure if we can ferret out all the false positives. Ideally, I'd still want to do it on the less side, but I get that that's architecturally difficult, and the calc solution is better than nothing.

Let's get all the specs passing and see what we've got then.

@confused-Techie
Copy link
Member Author

@savetheclocktower Thanks for checking it out! One nice thing also I've realized from doing it within Pulsar, is we get to create deprecations for it, allowing people to be more aware of the issue, and ideally fix it sooner. And it looks like, largely that calc() is to an extent what less uses under the hood. But yeah I'll work on getting the existing specs to pass. It seems mostly because of stylesheets being applied that are getting deprecations, causing the tests to fail. Which as far as I can tell, is mostly due to the last couple false positive situations

@confused-Techie
Copy link
Member Author

confused-Techie commented Jul 2, 2023

Looking at this further, since the last updates are so focused on ensuring we don't break existing or new style sheets, I've gone ahead and ensured users are able to opt out of this behavior completely, making this behavior configurable in settings.

Additionally, since the behavior this is based on of transforming selectors has been advertised to be removed for 7 years, I thought it may be a good idea to let users opt out of this behavior as well.

So that means, now there are two additional settings:

  • transformDeprecatedStyleSheetSelectors: Controls the existing behavior of changing the selectors of a style sheet. If enabled, style sheets will be modified.
  • transformDeprecatedStyleSheetMathExpressions: Controls this new behavior of changing mathematical expressions in style sheets. If enabled, style sheets will be modified.

An example of what this looks like now in our config:

image

Now, if for whatever reason users are finding this behavior to be undesirable, such as low resource systems having significantly increased startup time, or it's incorrectly modifying any specific style sheet, the option can be disabled until we are able to fix these issues upstream. Or if in the future, this behavior isn't needed for most users, then both can be disabled to increase startup performance.

@savetheclocktower
Copy link
Contributor

I like the configurability — if this ends up breaking in unforeseen ways, it gives users a workaround.

@confused-Techie
Copy link
Member Author

Thanks a ton for the approval! I'll give this a little time to allow anyone else that wants to take a look the time to do so, otherwise should be good to merge

@confused-Techie confused-Techie merged commit e892fd0 into master Jul 3, 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.

Remove request dependency
2 participants