Frontend JS Organization #84
Replies: 6 comments 5 replies
-
|
It might need to be: var Mage = Mage ?? {};
Mage.Adminhtml ??= {};
Mage.Adminhtml.Catalog ??= {};Or: window.Mage ??= {};
Mage.Adminhtml ??= {};
Mage.Adminhtml.Catalog ??= {};I created a test repo. Does this work with your IDE's autocomplete? There's only a few things defined: Mage.Adminhtml.Catalog.Category
Mage.Util.Dom.setMessagesDiv
Mage.Util.Dom.popWin
setMessagesDiv
popWinEdit: I tested in PHPStorm and it seems to work fine. |
Beta Was this translation helpful? Give feedback.
-
|
sorry man, I'll write soon :-) thanks for this in the meanwhile! |
Beta Was this translation helpful? Give feedback.
-
|
No problem, have been busy all day today anyway. |
Beta Was this translation helpful? Give feedback.
-
I was writing a longer answer with some considerations but at the end of the day I agree with your proposal 100% so.. :-D
yes, since probably a real class is needed anyway there
yes, since utilities probably are just a bunch of functions and don't need a real class that the use would have to instantiate
I this I'd like to have a compat.js file so that it can be excluded by the store developer, but also it's easier for us to just remove it at some point. Maybe I'd just have one single compat file instead of 3, since it will be just a few lines of code. but loading order could be a problem in some cases (when 3rd party loads a js in some weird way)... mmm...
nah I wouldn't like to have many different files, I think the way we described above it's perfectly fine :-) |
Beta Was this translation helpful? Give feedback.
-
One file sounds good to me.
This should work, I tested by loading this file before dom.js: const popWin = (...args) => Mage?.Util?.Dom?.popWin(...args);
const setMessagesDiv = (...args) => Mage?.Util?.Dom?.setMessagesDiv(...args);
Agreed, we definitely don't want super specific files with only like 2-3 functions defined in each. |
Beta Was this translation helpful? Give feedback.
-
|
A couple more examples: Depending on the file it might be cleaner to define like this, as long as no other file also puts members into the same namespace: Mage.Util.Dom = {
setMessagesDiv(message, type = 'success') {
console.log('Called Mage.Util.Dom.setMessagesDiv()');
},
popWin() {
console.log('Called Mage.Util.Dom.popWin()');
},
};Creating a subclass: Mage.Adminhtml.Catalog.Category = class {
constructor(config) {
console.log('Called new Mage.Adminhtml.Catalog.Category()');
}
foo() {
console.log('Category.foo()');
}
};
Mage.Adminhtml.Catalog.SubCategory = class extends Mage.Adminhtml.Catalog.Category {
constructor() {
super();
console.log('Called new Mage.Adminhtml.Catalog.SubCategory()');
}
foo() {
console.log('SubCategory.foo()');
}
} |
Beta Was this translation helpful? Give feedback.
-
|
for proper classes I think this method is ok (EG: configurable.js) but for utils I think |
Beta Was this translation helpful? Give feedback.
-
|
Few other points: 1. I think the util folder should actually be 2. For existing custom modules and themes with a layout xml file, do we need to provide any sort of mapping from old files to new? For example, a module wants to load |
Beta Was this translation helpful? Give feedback.
-
mmmm what if we remove the distinction between Mage and Maho and just migrate everything under the Maho namespace providing compatibility via the compat file? cause having js/mage and js/maho seems ok from our perspective but the end user it's not that great
mmm this is though, does it make sense to do that? will it just be a comment in a layout.xml that nobody would see? or should we document it in the release notes and that's it? |
Beta Was this translation helpful? Give feedback.
-
Yes, that sounds good to me!
I was thinking something more like this in public function addItem($type, $name, $params = null, $if = null, $cond = null, $referenceName = '*', $before = false)
{
if ($type === 'js') {
$name = match ($name) {
'varien/accordion.js' => 'maho/util/accordion.js',
'varien/form.js' => 'maho/util/form.js',
// ...
default => $name,
};
}What do you think about making a configuration option like "JavaScript compatibility mode" that controls both including |
Beta Was this translation helpful? Give feedback.
-
cause you're a genius, I wouldn't have thought about that
I love that idea actually! |
Beta Was this translation helpful? Give feedback.
-
Let's start a discussion about how to ultimately organize the js directory. Here's one possible idea.
Say we organize like this:
The main directories are:
js/lib- 3rd party librariesjs/mage/adminhtml- backend specific classes.js/mage/frontend- frontend specific classes.js/maho- new libraries we create.js/util- misc utility functions that can be used for either frontend / backend.To avoid namespace collisions, each file could define it's classes / methods like the below example. For example:
In
mage/adminhtml/catalog/category.js:In
util/dom.js:However, since that second example is totally backwards incompatible with code that expects
popWinin the global namespace, we can have a fewcompat.jsfiles.mage/adminhtml/compat.js- loaded on the backendmage/frontend/compat.js- loaded on the frontendutil/compat.js- loaded on bothThe last one might look something like this, so if
util/dom.jsis loaded, thenwindow.popWin()will be defined.Instead of
compat.js,util/dom.jscould also have just done this:Or:
The latter ideas are easier, but the former might give more control to a store owner who wants to disable
compat.jsloading to not pollute the global namespace.In a perfect world, we could be using es6 modules, but I don't think we're anywhere near that. This, or some other idea, would still be a step in the right direction.
The above is also just an example, we don't need to go crazy with splitting code into so many small chunks. So maybe the
utildir doesn't need to haveutil/dom.js,util/fetch.js,util/messages.js, etc. Maybe somewhere in between fully modular and the massivejs.jsfile we have now.Beta Was this translation helpful? Give feedback.
All reactions