-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
To @redmunds. Seems like there's some potential testing risk - please assess. Also note that there's a spurious CM SHA commit in here. |
Also, as a general rule, we only really want to update stuff like this if there's a major win to be had from upgrading (i.e., it fixes a major bug that affects us, has major performance gains, etc.) If none of those apply, we shouldn't upgrade now, because there's a cost in testing and potential destabilization. |
I will remove the CM commit. |
@@ -1 +1 @@ | |||
Subproject commit 5ccfedfb0a22a95ec6e28f7475d1bac9c349ce82 | |||
Subproject commit 5581979d2c624d6c32342e50bb417749ad799ee7 |
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 don't think you intended to change the CodeMirror SHA, right?
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.
right
@redmunds all fixed. |
@WebsiteDeveloper On Mac, the time spent processing brackets.less dropped from 600ms to 535ms, which is >10% improvement. Nice. But, many unnecessary messages are written to the console. Can you get rid of these?
|
We could set the logLevel of less to errors. But that would also stop showing the performance data. |
@redmunds @WebsiteDeveloper Note that the speed improvement will not benefit most Brackets users -- only Brackets developers -- since the LESS is pre-compiled in release builds these days. |
@redmunds changes pushed |
@@ -46,7 +46,8 @@ | |||
|
|||
<!-- Pre-load third party scripts that cannot be async loaded. --> | |||
<!-- build:js thirdparty/thirdparty.min.js --> | |||
<script src="thirdparty/less-1.4.2.min.js"></script> | |||
<script src="config.js"></script> |
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'm not crazy about the idea of introducing a new top-level script include file just to set the logging level, and it creates an object in the global namespace. I'll have to see what the rest of the team thinks about that.
At the very least this file needs to be renamed to something more descriptive such as less-config.js
and moved to the thirdparty/
folder.
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 didn't like the idea either but as to the object in global namespace, less does that anyway.
I also thought of changing the default value in less itself.
The reason i put this into a file is that we can't include inline scripts for chromebook deployment.
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 will be ok. Please move and rename the new file to thirdparty/less-config.js.
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.
@WebsiteDeveloper I forgot to add your id, so maybe you missed my previous comment?
@redmunds sorry for my long response time, i have not had the time to work on this in the last few months. |
Looks good. Merging. |
Finally |
Updating less for improvements made since 1.4.2
See https://github.com/less/less.js/blob/master/CHANGELOG.md for more detailed information.