[core] Simplify/Cleanup `StyleManager`

confused-Techie commented 8 months ago

While investigating #930 I realized how complicated and unintuitive the StyleManager code was in the locations I needed to inspect.

This is mostly due to the fact that significant portions of the logic was doubled up, but then required safety to not overlap itself.

So I've gone ahead and applied some refactoring and simplification of the code to make it easier to read, and ideally allow a follow up PR that resolves the issue mentioned above.

Which to be clear, this PR does not resolve the issue mentioned above. Only cleans up the code to make that easier, and of course try to make things more readable.

In this PR:

The functions upgradeDeprecatedSelectorsForStyleSheet and upgradeDeprecatedMathUsageForStyleSheet have been removed in favor of the function upgradeStyleSheet which accepts a callback parameter of a function to actually call, allowing that single function to do what the both of these did.
Within addStyleSheet I've gone ahead and removed all duplicated logic, instead opting for two variables textContent and deprecationMessages to track the current state of the styleElement.textContent and this.deprecationsBySourcePath values respectively.

But otherwise this PR should result in zero change in actual behavior, only making things hopefully more readable.

Spiker985 commented 7 months ago

One comment: do you know if we need to shim the upgradeDeprecatedSelectorsForStyleSheet and upgradeDeprecatedMathUsageForStyleSheet calls or are those only internal?

And do we also want to add Grim deprecate messages for those functions, on the off-chance that something somewhere uses it?

confused-Techie commented 7 months ago

@Spiker985 I'm rather confident that those functions were only ever used internally. I mean upgradeDepercatedSelectorsForStyleSheet isn't even directly called in the specs.

Searching the entire repository brings a total of two hits for upgradeDepercatedSelectorsForStyleSheet and they both exist in the style-manager. As for upgradeDeprecatedMathUsageForStyleSheet I'm happy to say we have no worries there since I implemented it not to long ago.

So I don't think we have anything to worry about tbh

confused-Techie commented 7 months ago

Thanks @DeeDeeG appreciate you taking a look! Do we wanna wait for the new set of specs to pass or do we feel confident with the last run prior to your suggestion?

DeeDeeG commented 7 months ago

Either way is fine with me! (Leaning slightly toward merge without waiting, since it's Release day and the change was intended to be pretty trivial. Hopefully I didn't put a fatal typo in my very brief code suggestion, but if I did that's my fault and I'll fix it some way.)

EDIT: For the record, the :x: above reflects cancelled tasks, not failed ones. (At least as of when I'm typing this.) I suppose GitHub Actions figured there'd be no point in continuing to run specs on a PR that already merged, canceled some tasks, and those show as a red "x" status.

DeeDeeG commented 7 months ago

Commit with the fix is being included in the version bump PR for 1.116.0: https://github.com/pulsar-edit/pulsar/pull/977.

(As it was explained to me: This PR had an old function name left in place, instead of the new name, which broke something only specs need/use. Unclear why it passed CI in the first place, only to then start failing on any subsequent test runs. (????) But is fixed in that commit, so it seems so far. Editor tests are passing for that branch with the fix. 👍)

pulsar-edit / pulsar

[core] Simplify/Cleanup `StyleManager` #959