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

0009 - Expose js core modules to the window object to provide an interface to every developers #12

Merged
merged 11 commits into from Aug 28, 2020

Conversation

NeOMakinG
Copy link

@NeOMakinG NeOMakinG commented Aug 20, 2020

Questions Answers
Description? We need to decide about a proper workflow of exposing core js modules to every developers using a global object such as window
Type? improvement
BC breaks? no
Deprecations? no

Vote

Maintainer Vote
@eternoendless
@PierreRambaud
@matks
@jolelievre
@Progi1984
@matthieu-rolland
@atomiix
@NeOMakinG
@sowbiba

@NeOMakinG NeOMakinG changed the title Initialize the ADR Expose js core modules to the window object to provide an interface to every developers Aug 20, 2020
0005-expose-js-modules-using-window-variable.md Outdated Show resolved Hide resolved
0005-expose-js-modules-using-window-variable.md Outdated Show resolved Hide resolved
0005-expose-js-modules-using-window-variable.md Outdated Show resolved Hide resolved
0005-expose-js-modules-using-window-variable.md Outdated Show resolved Hide resolved
0005-expose-js-modules-using-window-variable.md Outdated Show resolved Hide resolved
0005-expose-js-modules-using-window-variable.md Outdated Show resolved Hide resolved
0005-expose-js-modules-using-window-variable.md Outdated Show resolved Hide resolved
0005-expose-js-modules-using-window-variable.md Outdated Show resolved Hide resolved
Valentin Szczupak and others added 3 commits August 20, 2020 17:11
@eternoendless eternoendless changed the title Expose js core modules to the window object to provide an interface to every developers 0009 - Expose js core modules to the window object to provide an interface to every developers Aug 21, 2020
0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
translatableInput: {
init() {
if (translatableInput === null) {
translatableInput = new TranslatableInput();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must be sure the dom is loaded before running it, otherwise, if a module uses it, it will not trigger it on all input.
(Or we maybe improve this component to run it dynamically 🤔

@eternoendless
Copy link
Member

Here's what we discussed:

All PrestaShop components are bundled together and its code is available in all pages. Each controller decides which components it chooses to initialize.

There's not one, but two global variables: window.PS and window.prestashop. The first one is a namespace, the second one contains objects.

The PS namespace will contain classes like this PS.Component.SomeComponent. If you want to get a new instance of SomeComponent, you call new PS.Component.SomeComponent(...params)

The prestashop object contains two things: initComponents() and components.

prestashop.initComponents() is a factory method that receives an array of component names like this: ['PS.Component.SomeComponent', 'PrestaShop.Component.SomeOtherComponent']. It will create instances of the received classnames using default parameters and will store those instances in prestashop.components. So that when you create a PS.Component.SomeComponent then you will have an instance of it available in prestashop.components.someComponent.

Since you have access to both constructors and components, developers are free to choose how to initialize and control their components.

If you don't want to initialize a given component with default parameters, you can always call new PS.Component.SomeComponent(...myOwnParameters). You can even add it to the list of global components: prestashop.components.someComponent = new PS.Component.SomeComponent(...).

If you need to apply some mutation to an already initialized component, you just get the global instance: prestashop.components.someComponent.doSomething(...).

@PierreRambaud
Copy link
Contributor

Here's what we discussed:

All PrestaShop components are bundled together and its code is available in all pages. Each controller decides which components it chooses to initialize.

There's not one, but two global variables: window.PS and window.prestashop. The first one is a namespace, the second one contains objects.

The PS namespace will contain classes like this PS.Component.SomeComponent. If you want to get a new instance of SomeComponent, you call new PS.Component.SomeComponent(...params)

The prestashop object contains two things: initComponents() and components.

prestashop.initComponents() is a factory method that receives an array of component names like this: ['PS.Component.SomeComponent', 'PrestaShop.Component.SomeOtherComponent']. It will create instances of the received classnames using default parameters and will store those instances in prestashop.components. So that when you create a PS.Component.SomeComponent then you will have an instance of it available in prestashop.components.someComponent.

Since you have access to both constructors and components, developers are free to choose how to initialize and control their components.

If you don't want to initialize a given component with default parameters, you can always call new PS.Component.SomeComponent(...myOwnParameters). You can even add it to the list of global components: prestashop.components.someComponent = new PS.Component.SomeComponent(...).

If you need to apply some mutation to an already initialized component, you just get the global instance: prestashop.components.someComponent.doSomething(...).

We should use only one global variable, don't forget when you put data in the window object, they are registered as global, this means: window.PS = PS, so having two globals variables is a risk, a module can delete one of them :/

Why not

window.prestashop.Component.SomeComponent;
window.prestashop.components;

@eternoendless
Copy link
Member

I proposed using two variables to make it clear that one is a namespace and the other a collection.

so having two globals variables is a risk, a module can delete one of them

I can't see it would be less risky to have a single variable having into account that modules can do whatever they like :)

@PierreRambaud
Copy link
Contributor

I proposed using two variables to make it clear that one is a namespace and the other a collection.

so having two globals variables is a risk, a module can delete one of them

I can't see it would be less risky to have a single variable having into account that modules can do whatever they like :)

The more globals variables you have, the more you have to lose :trollface:
it can also be confusing, PS looks outdated compared to prestashop, people can think it's a deprecated variable or regarding the ecosystem, think it's an issue 😅

@eternoendless
Copy link
Member

I wanted to make a clear difference between the two:

prestashop.Component.SomeComponent   <-- the component class
prestashop.components.someComponent  <-- the component instance

The difference is too subtle there, I'm sure people will mix them up all the time.

How about this:

prestashop.component.SomeComponent    <-- the class
prestashop.allComponents.someComponent <-- the instance

// alternatives
- prestashop.global.someComponent
- prestashop.instance.someComponent

@NeOMakinG
Copy link
Author

I'm in favor of

- prestashop.component.SomeComponent
- prestashop.instance.someComponent

It's way clearer tbh

@jolelievre
Copy link
Contributor

I also agree with @NeOMakinG naming proposition. And I would add one last modif related to this naming and the initComponents function, we could simplify it so that it is used this way:

initComponents(['TranslatableInput']);

instead of using a long namespace chain, since every component classes will be stored in window.prestashop.component anyway it will be easy to access them without some complex parsing of a namespace

This also avoid declaring the namespaced classes as global, just in the prestashop component list is enough, which also allows modules to easily add their own component

Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just typo and one precision to avoid misunderstanding, but otherwise I validate this and vote yes.

0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
0009-expose-js-components-using-window-variable.md Outdated Show resolved Hide resolved
SZCZUPAK Valentin and others added 3 commits August 28, 2020 15:35
Co-authored-by: Jonathan Lelievre <jo.lelievre@gmail.com>
Co-authored-by: Jonathan Lelievre <jo.lelievre@gmail.com>
Co-authored-by: Jonathan Lelievre <jo.lelievre@gmail.com>
@matks
Copy link
Contributor

matks commented Aug 28, 2020

Opening the voting phase 🎉

@PierreRambaud
Copy link
Contributor

LGTM

@NeOMakinG
Copy link
Author

LGTM too

@jolelievre
Copy link
Contributor

LGTM too:
sorry last modif though 😅
https://github.com/PrestaShop/ADR/pull/12/files#r479310822

@matthieu-rolland
Copy link

LGTM

Co-authored-by: Jonathan Lelievre <jo.lelievre@gmail.com>
@matks
Copy link
Contributor

matks commented Aug 28, 2020

As we have reached majority, this ADR is accepted.

@matks matks merged commit 3de1848 into PrestaShop:master Aug 28, 2020
@matks
Copy link
Contributor

matks commented Aug 28, 2020

Thank you everybody !


What could become harder :

- It will require more knowledge about the project. Before, you needed to know that TranslatableInput exist, while you'll need to also now that they are global and don't require to be imported at all.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... also know that ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch 😉

Will you submit a PR or I do it ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn

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.

None yet

7 participants