Deprecate lodash.mixxx.js, and script.deepMerge#13460
Conversation
| setLayer(newLayer, reconnectComponents) { | ||
| if (reconnectComponents !== false) { | ||
| reconnectComponents = true; | ||
| } | ||
| if (reconnectComponents === true) { | ||
| this.forEachComponent(function(component) { | ||
| component.disconnect(); | ||
| }); | ||
| } | ||
|
|
||
| Object.assign(this, newLayer); | ||
|
|
||
| if (reconnectComponents === true) { | ||
| this.forEachComponent(function(component) { | ||
| component.connect(); | ||
| component.trigger(); | ||
| }); | ||
| } | ||
|
|
||
| }, |
There was a problem hiding this comment.
I decided to create a copy with the new behavior so that it is more explicit that the implementation changes.
| }; | ||
|
|
||
| /** @deprecated Use {@link Object.assign} instead */ | ||
| script.deepMerge = deepMerge; |
There was a problem hiding this comment.
Damn this file need a big refactoring…
There was a problem hiding this comment.
Everything is declared using var sothat variables endup in the global scope. script should be declared as such:
(function(global) {
const script = Object.freeze({
LIBRARY_COLUMNS: Object.freeze({ … }),
debug() { … }
});
global.script = script;
})(this)That would improve readability IMHO.
Maybe even globalThis can be used? I need to check if implemented in QJSEngine.
Edit: well, no globalThis is not defined. I expected this. Sad.
There was a problem hiding this comment.
Also print() is still used everywhere (could be remove in 2.60 too).
There was a problem hiding this comment.
I assume you mean global.script = ...? But yeah, employing that pattern would probably be better, even though I don't find it to improve readability particularly.
There was a problem hiding this comment.
I assume you mean
global.script = ...?
I yes, sorry, let me edit.
There was a problem hiding this comment.
Damn this file need a big refactoring…
Are you aware of #11625? Help to fix the build failure there is appreciated.
| applyLayer: function(newLayer, reconnectComponents) { | ||
| console.warn("Components.applyLayer is deprecated; use Component.setLayer instead"); |
There was a problem hiding this comment.
applyLayer is used extensively. Do you think its feasible to transition some code to setLayer in certain mappings?
There was a problem hiding this comment.
In this PR? Sure! Every mapping or just one or two?
There was a problem hiding this comment.
not sure. I'd say every one where you can be sure that the change to Object.assign does not cause regressions. Ideally any hardware you have on hand to test/verify that it didn't break anything.
There was a problem hiding this comment.
Uh. I only have 2 Denon DN-S3700 here, sorry.
There was a problem hiding this comment.
Do you have an idea aboutwhat are the two most used controllers? I propose to start by them and them ask the community to test. And rollback on any issue.
There was a problem hiding this comment.
not sure. I'm guessing many pioneer controllers. Apart from that I could test on my NS6II and there are several people in the mixxx team that have a traktor S4MK3 (though that uses its own ComponentsJS implementation IIRC).
There was a problem hiding this comment.
We could just switch it for all mappings before a beta and see if we receive complaints during the beta phase... Thats what betas are for IMO.
There was a problem hiding this comment.
So we switch the mappings in a seperate PR ?
| */ | ||
| ;(function() { | ||
| const DEPRECATION_MSG = ( | ||
| "Lodash is deprecated and will be removed in Mixxx 2.6.0; refer to the page " + |
There was a problem hiding this comment.
| "Lodash is deprecated and will be removed in Mixxx 2.6.0; refer to the page " + | |
| "Lodash is deprecated and will be removed in a future release of Mixxx; refer to the page " + |
I'm in favor of deprecating Lodash and printing a warning, but we should not remove it to early. Third party mappings might break due to this change.
There was a problem hiding this comment.
Done. Thank you.
christophehenry
force-pushed
the
depreciate-lodash
branch
from
2f6114b to
e5f393e
Compare
|
@Swiftb0y @JoergAtGithub Uh wait. There's one more typo. Edit: Done. |
christophehenry
force-pushed
the
depreciate-lodash
branch
from
e5f393e to
f1d3287
Compare
christophehenry
force-pushed
the
depreciate-lodash
branch
from
f1d3287 to
1be0fe7
Compare
| } | ||
| }; | ||
|
|
||
| /** @deprecated Use {@link Object.assign} instead */ |
There was a problem hiding this comment.
This @deprecated is not recognized. I think it only works before the function definition.
There was a problem hiding this comment.
Isn't it? What IDE are you on? This is correctly interpreted on CLion:
There was a problem hiding this comment.
It neither work with VisualStudio nor VS Code
There was a problem hiding this comment.
What if i write:
/**
* Deeply merges 2 objects (Arrays and Objects only, not Map for instance).
* @param target {object | Array} Object to merge source into
* @param source {object | Array} Object to merge into source
* @deprecated Use {@link Object.assign} instead
*/
script.deepMerge = function(target, source) {
console.warn("script.deepMerge is deprecated; use Object.assign instead");
…
}There was a problem hiding this comment.
Than it warns in line script.deepMerge = deepMerge; that deepMerge is deprecated.
There was a problem hiding this comment.
My last commit should fix this. Can you test (I don't know how to tell to VSCode "there are global definitions in these files, plz source")?
| return obj !== null && typeof obj === "object" && obj.constructor.name === "Object"; | ||
| }; | ||
|
|
||
| script.isSimpleObject = isSimpleObject; |
There was a problem hiding this comment.
I don't think this should be exported.
|
I think it's ready for a merge, isn't it? |
|
I adapted the changes in Thank you! |
|
Oh that was fast! Thank you! |
3 participants
As decided in #13457.