You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flagon.apache.org by GitBox <gi...@apache.org> on 2023/01/02 17:07:34 UTC
[GitHub] [incubator-flagon-useralejs] UncleGedd opened a new pull request, #331: Revisit composedPath update
UncleGedd opened a new pull request, #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331
This PR revisits #323 to discuss lingering talking points including:
- Testing strategy for `buildPath`
- Deprecating `initEvent1
- Refactor `buildPath` how @Jyyjy originally intended
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] UncleGedd commented on a diff in pull request #331: WIP: Revisit composedPath update
Posted by GitBox <gi...@apache.org>.
UncleGedd commented on code in PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331#discussion_r1060137498
##########
test/packageLogs_spec.js:
##########
@@ -313,28 +313,15 @@ describe('packageLogs', () => {
describe('buildPath', () => {
it('builds a path', () => {
new JSDOM(``)
+ let actualPath
const document = window.document;
const ele = document.createElement('div');
- const evt = document.createEvent('CustomEvent');
- evt.initEvent('testEvent', true, true);
+ const evt = new window.Event('CustomEvent', {bubbles: true, cancelable: true})
document.body.appendChild(ele);
+ ele.addEventListener('CustomEvent', e => actualPath = buildPath(e))
ele.dispatchEvent(evt);
- expect(buildPath(evt)).to.deep.equal(['div', 'body', 'html']);
- });
-
- it('defaults to path if available', () => {
- new JSDOM(``)
- const document = window.document;
- const ele = document.createElement('div');
- const evt = document.createEvent('CustomEvent');
- document.body.appendChild(ele);
- evt.initEvent('testEvent', true, true);
- ele.dispatchEvent(evt);
- evt.composedPath = function() {
- let ele = evt.target;
- return [ele, ele.parentElement, ele.parentElement.parentElement];
- };
- expect(buildPath(evt)).to.deep.equal(['div', 'body', 'html']);
+ const expectedPath = ['div', 'body', 'html', "#document", "Window"]
Review Comment:
Noting here that the `expectedPath` now includes `"#document"` and `"Window"`. This is expected behavior with `composedPath`, see screenshot from Chrome dev tools below:
<img width="554" alt="Screen Shot 2023-01-02 at 11 14 13 AM" src="https://user-images.githubusercontent.com/42304551/210262096-5436c066-57a9-481c-906f-85069f0e8fae.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] UncleGedd commented on pull request #331: WIP: Revisit composedPath update
Posted by GitBox <gi...@apache.org>.
UncleGedd commented on PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331#issuecomment-1369100265
Made 2 updates thus far:
1. Refactored the `buildPath` function how @Jyyjy orignally intended ([reference](https://github.com/apache/incubator-flagon-useralejs/pull/323#issuecomment-1365557602)) including fixing the unit test
2. Deleted the `defaults to path if available` unit test. Someone check me on this, but I'm not sure this test is relevant anymore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] UncleGedd commented on a diff in pull request #331: WIP: Revisit composedPath update
Posted by GitBox <gi...@apache.org>.
UncleGedd commented on code in PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331#discussion_r1060137498
##########
test/packageLogs_spec.js:
##########
@@ -313,28 +313,15 @@ describe('packageLogs', () => {
describe('buildPath', () => {
it('builds a path', () => {
new JSDOM(``)
+ let actualPath
const document = window.document;
const ele = document.createElement('div');
- const evt = document.createEvent('CustomEvent');
- evt.initEvent('testEvent', true, true);
+ const evt = new window.Event('CustomEvent', {bubbles: true, cancelable: true})
document.body.appendChild(ele);
+ ele.addEventListener('CustomEvent', e => actualPath = buildPath(e))
ele.dispatchEvent(evt);
- expect(buildPath(evt)).to.deep.equal(['div', 'body', 'html']);
- });
-
- it('defaults to path if available', () => {
- new JSDOM(``)
- const document = window.document;
- const ele = document.createElement('div');
- const evt = document.createEvent('CustomEvent');
- document.body.appendChild(ele);
- evt.initEvent('testEvent', true, true);
- ele.dispatchEvent(evt);
- evt.composedPath = function() {
- let ele = evt.target;
- return [ele, ele.parentElement, ele.parentElement.parentElement];
- };
- expect(buildPath(evt)).to.deep.equal(['div', 'body', 'html']);
+ const expectedPath = ['div', 'body', 'html', "#document", "Window"]
Review Comment:
Noting here that the `expectedPath` now includes `"#document"` and `"Window"`. This is expected behavior with `composedPath`, see screenshot from Chrome dev tools below:
<img width="554" alt="Screen Shot 2023-01-02 at 11 14 13 AM" src="https://user-images.githubusercontent.com/42304551/210262096-5436c066-57a9-481c-906f-85069f0e8fae.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] Jyyjy commented on pull request #331: Revisit composedPath update
Posted by GitBox <gi...@apache.org>.
Jyyjy commented on PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331#issuecomment-1370162818
These are the changes that I intended. Thanks @UncleGedd!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] UncleGedd commented on pull request #331: Revisit composedPath update
Posted by GitBox <gi...@apache.org>.
UncleGedd commented on PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331#issuecomment-1369856009
Ready for review! I added a Cypress test as well to test the `buildPath` behavior in a real browser. Also, refactored an old test that wasn't working properly (`"does not send logs on page exit when config is off"`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] poorejc commented on pull request #331: Revisit composedPath update
Posted by "poorejc (via GitHub)" <gi...@apache.org>.
poorejc commented on PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331#issuecomment-1407750383
@UncleGedd this is a really elegant update--I'm in love with the Cypress tests! Very nice. Tests well. Merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] asfgit merged pull request #331: Revisit composedPath update
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit merged PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] UncleGedd commented on pull request #331: Revisit composedPath update
Posted by GitBox <gi...@apache.org>.
UncleGedd commented on PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331#issuecomment-1380796158
@poorejc I forget our process. Do you want to merge this into `test` first or go directly into `master`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-flagon-useralejs] poorejc commented on a diff in pull request #331: Revisit composedPath update
Posted by "poorejc (via GitHub)" <gi...@apache.org>.
poorejc commented on code in PR #331:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/331#discussion_r1083245162
##########
test/packageLogs_spec.js:
##########
@@ -313,28 +313,15 @@ describe('packageLogs', () => {
describe('buildPath', () => {
it('builds a path', () => {
new JSDOM(``)
+ let actualPath
const document = window.document;
const ele = document.createElement('div');
- const evt = document.createEvent('CustomEvent');
- evt.initEvent('testEvent', true, true);
+ const evt = new window.Event('CustomEvent', {bubbles: true, cancelable: true})
document.body.appendChild(ele);
+ ele.addEventListener('CustomEvent', e => actualPath = buildPath(e))
ele.dispatchEvent(evt);
- expect(buildPath(evt)).to.deep.equal(['div', 'body', 'html']);
- });
-
- it('defaults to path if available', () => {
- new JSDOM(``)
- const document = window.document;
- const ele = document.createElement('div');
- const evt = document.createEvent('CustomEvent');
- document.body.appendChild(ele);
- evt.initEvent('testEvent', true, true);
- ele.dispatchEvent(evt);
- evt.composedPath = function() {
- let ele = evt.target;
- return [ele, ele.parentElement, ele.parentElement.parentElement];
- };
- expect(buildPath(evt)).to.deep.equal(['div', 'body', 'html']);
+ const expectedPath = ['div', 'body', 'html', "#document", "Window"]
Review Comment:
@UncleGedd AWESOME. Thanks, Gedd. Was planning some maintenance next week... I'll beat on this a bit and report back!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@flagon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org