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 2022/12/15 00:46:22 UTC
[GitHub] [incubator-flagon-useralejs] Jyyjy opened a new pull request, #323: Composed path
Jyyjy opened a new pull request, #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323
This PR is to solve this issue: https://github.com/apache/incubator-flagon-useralejs/issues/311
This may not be the best solution, but it is the one that matches the behavior of the old code the best. In the old code, .path only existed in chromium browsers, so we needed to sometimes manually build the path. This created the need for two test cases. composedPath() is universal, so we never have to manually build the path. But I decided to keep the manual path construction and the test case for it.
Another quirk of the old code is that Event in the testcases is not the same as Event in the source. And the Event in the testcases does not populate the path correctly. That's why in both the old and new code the testcase forces the path to a value.
A better solution for buildPath would look something like this:
`export function buildPath(e) {
if (typeof e == "Event") {
let path = e.composedPath();
return selectorizePath(path);
}
return null;
}`
But I couldn't figure out how to properly test that. So given that the end behavior would be the same, and this is a time sensitive issue, I decided to go with the least invasive solution.
--
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 #323: Composed path
Posted by GitBox <gi...@apache.org>.
poorejc commented on PR #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323#issuecomment-1365562374
merged with test, will test some more and then see if we want to merge with 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] Jyyjy commented on pull request #323: Composed path
Posted by GitBox <gi...@apache.org>.
Jyyjy commented on PR #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323#issuecomment-1355412304
> @Jyyjy -- are there follow ups to be potentially done [ in the future ,with more time ]?
>
> Said another way: sounds like this addresses an urgent need, what cleanup/changes are worthy of consideration for cleanup?
There's a few potential follow up changes I would make/suggest:
1. The "defaults to path if available" testcase sets the path (or compossedPath() in my pr) to a value rather than testing the path that is generated by the dispatched event.
2. initEvent(), which is used a few times throughout the testcases, is deprecated and should be replaced with event constructors.
3. There's no need to manually build a path or for the "builds a path" testcase because composedPath() is universally supported.
--
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] brucearctor commented on pull request #323: Composed path
Posted by GitBox <gi...@apache.org>.
brucearctor commented on PR #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323#issuecomment-1352442982
I don't have the context to think about reviewing this currently ( am on vacation, would want to dig into code much more deeply, ahead of an 'actual' approval). BUT, am approving, so that the workflow can run. Hoping someone else can actually review for code standards/functionality.
--
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 #323: Composed path
Posted by GitBox <gi...@apache.org>.
poorejc commented on PR #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323#issuecomment-1365578401
Looks like paths are getting appropriately parsed:
```
target: 'button#test_button',
path: [
'button#test_button',
'div.container',
'body',
'html',
'#document',
'Window'
],
```
I tested against freshly built artifacts--both base userale.js script and browser extension are logging correctly to the example server.
Presently am rebuilding my ELK cluster. Once it's up and running, I'll test and log to ELK and see if paths still look good. If so, I'm happy pushing to Master. Good add @Jyyjy!
--
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 #323: Composed path
Posted by GitBox <gi...@apache.org>.
poorejc commented on PR #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323#issuecomment-1365557602
@Jyyjy Thanks! This is great and timely. I'm going to do some additional integration tests on this, and see if everything looks solid all the way through the pipeline.
RE: "A better solution for buildPath would look something like this:
`export function buildPath(e) {
if (typeof e == "Event") {
let path = e.composedPath();
return selectorizePath(path);
}
return null;
}`
But I couldn't figure out how to properly test that. "
Let's get that up to @dev and see what someone like @UncleGedd thinks--he helped to do a massive overhaul of our test framework not too long ago.
Again, many thanks.
--
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] brucearctor commented on pull request #323: Composed path
Posted by GitBox <gi...@apache.org>.
brucearctor commented on PR #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323#issuecomment-1352445015
@Jyyjy -- are there follow ups to be potentially done [ in the future ,with more time ]?
Said another way: sounds like this addresses an urgent need, what cleanup/changes are worthy of consideration for cleanup?
--
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 #323: Composed path
Posted by GitBox <gi...@apache.org>.
UncleGedd commented on PR #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323#issuecomment-1367637768
👍 In the next few days I'll check this out and see if we can't bolster the testing with the original solution
--
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 #323: Composed path
Posted by GitBox <gi...@apache.org>.
asfgit merged PR #323:
URL: https://github.com/apache/incubator-flagon-useralejs/pull/323
--
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