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

[LoAF] Use sourceLine and sourceColumn instead of sourceCharPosition for attribution #16

Open
Miz85 opened this issue Jun 26, 2024 · 10 comments

Comments

@Miz85
Copy link

Miz85 commented Jun 26, 2024

Source maps use line and column in order to map generated code to the matching line and column in the original file. It would make it a lot easier to link from the code attribution in a LoAF to the original source code if the LoAF could include sourceLine and sourceColumn.

Otherwise if you have some thoughts about how to achieve this using the sourceCharPosition I would be very interested. Or even more information about what that position refers to e.g. do line breaks count as 1 char?

@mmocny
Copy link
Contributor

mmocny commented Jun 27, 2024

I agree that this would be a lot more convenient for developers to see line/column instead of char position. In an earlier prototype that is actually how it worked.

However-- Noam noticed that this was a perf regression, because it requires parsing the text (in order to find all the newline characters). This isn't something that happens typically and so it is expensive to do automatically.


I wonder if it could be possible to request this type of parsing explicitly, after the LoAF fires, only once we know there is a client that is interested?

@Miz85
Copy link
Author

Miz85 commented Jun 28, 2024

Thanks for the reply!

For more context, I'm sending the LoAF to an observability tool for later use and I'm trying to link the sourceCharPosition to the matching development file.
Agreed that natively paying that cost for every LoAF is probably not great. A way to opt-in to it would be a good alternative.

I tried to replicate this in user space by using fetch and only-if-cached request in order to retrieve the source file content if it exists in the browser cache. I then compute the sourceLine and sourceColumn from the sourceCharPosition provided by the LoAF and it seems to work well.

However it would be even better with the opt-in API you're suggesting.

@mmocny
Copy link
Contributor

mmocny commented Jun 28, 2024 via email

@Miz85
Copy link
Author

Miz85 commented Jun 29, 2024

yes you're absolutely right! However I'm working on an observability tool and I want to collect LoAFs for our users and link to their source code automatically.

So my constraints are:

  • I don't have access to the minified file on the server side
  • I do have access to source maps because users have a way to send them to us through an API

This is why I did that way. However it's good enough for a POC but I'm probably gonna hit CORS issues. If the opt-in API you were suggesting existed, I could let our users opt-in to that computation and accept the performance hit as a trade off to being able to monitor LoAFs.

@Miz85
Copy link
Author

Miz85 commented Jul 4, 2024

@mmocny Coming back to your idea about performing the parsing only once we know there is a client that is interested. What would be a good way to advocate for that and see if it could actually get included down the line?

@mmocny
Copy link
Contributor

mmocny commented Jul 4, 2024

I think you just did :P

@noamr for thoughts.

@noamr
Copy link
Collaborator

noamr commented Jul 4, 2024

I think this is better done in a service-worker than in the browser... Put the service worker between the document and the server that gives you the script, and tee the response stream to some processor that returns an array of line breaks. It should be trivial to create a get_line_and_col(source_char_position, array_of_line_breaks) function based on a similar function in chromium code.

@Miz85
Copy link
Author

Miz85 commented Jul 8, 2024

Hey @noamr, thank you so much for your reply! Alright, sounds like a good path forward I'm going to give it a shot.

@Miz85
Copy link
Author

Miz85 commented Nov 15, 2024

Hi again! I found a way around this in the end but I do have a question on what sourceCharacterPosition represents.
Especially in the context of SPAs e.g. a React app it can point to a position in a minified file. When successfully unminifying it, it sometimes points to import statements for ex. which seems confusing.

So my question on this would be, is the sourceCharPosition pointing to beginning of a task that is executed by the browser? Meaning that in order to understand the performance issue we would need to also know the rest of the code executed in that task.

@noamr
Copy link
Collaborator

noamr commented Nov 15, 2024

Hi again! I found a way around this in the end but I do have a question on what sourceCharacterPosition represents. Especially in the context of SPAs e.g. a React app it can point to a position in a minified file. When successfully unminifying it, it sometimes points to import statements for ex. which seems confusing.

So my question on this would be, is the sourceCharPosition pointing to beginning of a task that is executed by the browser? Meaning that in order to understand the performance issue we would need to also know the rest of the code executed in that task.

It points to the "entry point" function, see key point in https://developer.chrome.com/docs/web-platform/long-animation-frames#better-attribution

Also see issue #3 which we're currently working on.

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

No branches or pull requests

3 participants