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

[PLAYER-25] adding api to track event #84

Merged
merged 4 commits into from
Jul 26, 2024

Conversation

jparez
Copy link
Contributor

@jparez jparez commented Jul 18, 2024

Description

Store

Refacto store to subscribe only part of store change. Most likely React useEffect hook, we can now supply an array of dependencies. If no array is supply then the callback will be trigger on each store change

this callback will be trigger only on isKeyboardEventsEnabled change

   this.instance.store.subscribe(
            ({isKeyboardEventsEnabled}) => {
                ...
            },
            ['isKeyboardEventsEnabled'],
        );

nested object

   this.instance.store.subscribe(
            ({isKeyboardEventsEnabled}) => {
                ...
            },
            ['overlay.widgetOpened'],
        );

Trigger on all change

   this.instance.store.subscribe(
            ({isKeyboardEventsEnabled}) => {
                ...
            }
        );

new feature

Adding functions to ApiManager to track events

Events currently tracked

  • opengapps installed
  • gamepad plug / unplug
  • widget opened / closed

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I've read & comply with the contributing guidelines
  • I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • I have made corresponding changes to the documentation (README.md).
  • I've checked my modifications for any breaking changes.

@jparez jparez changed the base branch from main to dev/PLAYER-19-keybinding-post-refacto-dialog July 18, 2024 15:03
@jparez jparez force-pushed the dev/PLAYER-25-adding-api-to-track-event branch from be69843 to 31b2790 Compare July 24, 2024 08:56
@jparez jparez requested a review from pgivel July 24, 2024 09:08
Base automatically changed from dev/PLAYER-19-keybinding-post-refacto-dialog to main July 25, 2024 12:28
@jparez jparez force-pushed the dev/PLAYER-25-adding-api-to-track-event branch 2 times, most recently from c60e513 to 563f577 Compare July 25, 2024 13:02
@jparez jparez force-pushed the dev/PLAYER-25-adding-api-to-track-event branch from 563f577 to af7ea80 Compare July 25, 2024 15:15
Copy link
Contributor

@pgivel pgivel left a comment

Choose a reason for hiding this comment

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

this is promising. Nice architecture as always

src/plugins/util/OverlayPlugin.js Show resolved Hide resolved
payload: {
category: 'opengapps',
type: 'installed',
name: 'true',
Copy link
Contributor

Choose a reason for hiding this comment

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

le name: 'true' me paraît étrange. ça devrait pas être un truc genre value: 'true' ou details: 'true' ?

src/store/index.js Show resolved Hide resolved
Comment on lines +36 to +69
const findChangedKeys = (newState, oldState, path = []) => {
let changedKeys = [];

const isObject = (obj) => obj && typeof obj === 'object';

for (const key in newState) {
const fullPath = [...path, key].join('.');

if (!Object.prototype.hasOwnProperty.call(oldState, key)) {
changedKeys.push(fullPath);
} else if (isObject(newState[key]) && isObject(oldState[key])) {
if (Array.isArray(newState[key]) && Array.isArray(oldState[key])) {
if (newState[key].length !== oldState[key].length) {
changedKeys.push(fullPath);
} else {
let arrayHasChanged = false;
for (let i = 0; i < newState[key].length; i++) {
if (newState[key][i] !== oldState[key][i]) {
arrayHasChanged = true;
}
}
if (arrayHasChanged) {
changedKeys.push(fullPath);
}
}
} else {
changedKeys = changedKeys.concat(findChangedKeys(newState[key], oldState[key], [...path, key]));
}
} else if (newState[key] !== oldState[key]) {
changedKeys.push(fullPath);
}
}

return changedKeys;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice for readibility to refactor this function in order to avoid such deep nesting of conditions & loops. For example by extracting to a function the array check ?

Suggested change
const findChangedKeys = (newState, oldState, path = []) => {
let changedKeys = [];
const isObject = (obj) => obj && typeof obj === 'object';
for (const key in newState) {
const fullPath = [...path, key].join('.');
if (!Object.prototype.hasOwnProperty.call(oldState, key)) {
changedKeys.push(fullPath);
} else if (isObject(newState[key]) && isObject(oldState[key])) {
if (Array.isArray(newState[key]) && Array.isArray(oldState[key])) {
if (newState[key].length !== oldState[key].length) {
changedKeys.push(fullPath);
} else {
let arrayHasChanged = false;
for (let i = 0; i < newState[key].length; i++) {
if (newState[key][i] !== oldState[key][i]) {
arrayHasChanged = true;
}
}
if (arrayHasChanged) {
changedKeys.push(fullPath);
}
}
} else {
changedKeys = changedKeys.concat(findChangedKeys(newState[key], oldState[key], [...path, key]));
}
} else if (newState[key] !== oldState[key]) {
changedKeys.push(fullPath);
}
}
return changedKeys;
};
const arraysAreEqual = (firstArray, secondArray) => {
if (firstArray.length !== secondArray.length) {
return true;
}
for (let i = 0; i < firstArray.length; i++) {
if (firstArray[i] !== secondArray[i]) {
return true;
}
}
return false;
}
const findChangedKeys = (newState, oldState, path = []) => {
let changedKeys = [];
const isObject = (obj) => obj && typeof obj === 'object';
for (const key in newState) {
const fullPath = [...path, key].join('.');
if (!Object.prototype.hasOwnProperty.call(oldState, key)) {
changedKeys.push(fullPath);
} else if (isObject(newState[key]) && isObject(oldState[key])) {
if (Array.isArray(newState[key]) && Array.isArray(oldState[key]) && !arraysAreEqual(newState[key], oldState[key])) {
changedKeys.push(fullPath);
} else {
changedKeys = changedKeys.concat(findChangedKeys(newState[key], oldState[key], [...path, key]));
}
} else if (newState[key] !== oldState[key]) {
changedKeys.push(fullPath);
}
}
return changedKeys;
};

But there's probably ways to reduce even more the nesting.

@jparez jparez force-pushed the dev/PLAYER-25-adding-api-to-track-event branch from 29e029c to e099e07 Compare July 26, 2024 12:26
gulpfile.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should have been commited

@jparez jparez force-pushed the dev/PLAYER-25-adding-api-to-track-event branch from 6597c72 to 62e7c79 Compare July 26, 2024 12:58
@jparez jparez merged commit 5fd615c into main Jul 26, 2024
1 check passed
@jparez jparez deleted the dev/PLAYER-25-adding-api-to-track-event branch July 26, 2024 13:18
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.

2 participants