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

assets: Emit source maps #11042

Merged
merged 1 commit into from
Oct 7, 2024
Merged

assets: Emit source maps #11042

merged 1 commit into from
Oct 7, 2024

Conversation

PistonPercy
Copy link
Contributor

@PistonPercy PistonPercy commented Jul 19, 2024

Before clicking "Create"

  • Branch is derived from the latest master
  • Add the pins or archive_alpha_games label if this change will break existing games
  • Code passes linter with docker compose exec rack rubocop -a
  • Tests pass cleanly with docker compose exec rack rake

Implementation Notes

I had to rename deps because there was a name collision where the file was written to twice once in compile_lib and another time in combine and thus the corresponding source maps would get mangled.

Explanation of Change

This made it much easier to debug some issues. I was able to get ruby source code in the web inspector and see where exceptions where being thrown from.

Any Assumptions / Hacks

@vandamm
Copy link
Collaborator

vandamm commented Jul 21, 2024

I think source maps are only relevant in development, so you shouldn't add them if @precompiled is true

@PistonPercy PistonPercy requested a review from ollybh as a code owner July 22, 2024 05:03
@vandamm vandamm added the Core touches shared code not limited to one game or game family label Jul 23, 2024
@PistonPercy
Copy link
Contributor Author

It's mostly for development, but it doesn't hurt if it is in production. I guess we do need to have to change the deploy script to actual make it into production.

My ruby is pretty weak, but I'm attempting to refactor out all the sourcemap building logic into a helper (which is probably a good idea anyways) and then it'll easier to wrap it with conditionals.

@vandamm
Copy link
Collaborator

vandamm commented Jul 23, 2024

I can at least confirm it's working as intended and is quite useful

Screenshots Screenshot 2024-07-23 at 12 54 59 Screenshot 2024-07-23 at 12 55 18

@PistonPercy PistonPercy force-pushed the source_maps branch 2 times, most recently from 2270a42 to b2014dd Compare July 25, 2024 06:07
@PistonPercy
Copy link
Contributor Author

This isn't very well tested yet, but I'm not sure if I'll be able to get back to this for a few days. Just dropping in my most recent revision which does fix the double reading of builds and I think properly gates source map generation. My ruby is pretty weak and I'm not sure about the project conventions, so feel free to ask for fixes.

I had to rename deps because there was a name collision where the file was
written to twice once in compile_lib and another time in combine.

This made it much easier to debug some issues.
@PistonPercy
Copy link
Contributor Author

I ran make clean && docker-compose exec rack rake precompile and saw no source maps.

Then I ran make clean and loaded the minimal repro from #11020 and saw a backtrace.

When I looking at timings of individual files, it looks like it's very little overhead.

@ollybh
Copy link
Collaborator

ollybh commented Aug 25, 2024

I'd not come across source maps before. I've had a play around with the browser debugger with these maps added, and this does look like this is going to be very helpful. Trying to debug using the Opal-generated Javascript was borderline impossible, being able to set breakpoints and inspect values in the Ruby code is so much better.

@PistonPercy you've tagged me to review this pull request, but I don't understand the build system well enough to be able to do that. I'm sure that when @michaeljb has some time he'll be able to assess the changes that you've made.

One thing I don't follow is the different methods you've used for generating the source maps in compile_libs and combine. In the former you use Opal::Builder::source_map.map.to_json but in the latter you've taken a different approach with the SourceMap class. What's the difference between these? The Opal docs are not great, so I don't really follow what's happening in these two methods.

Copy link
Collaborator

@michaeljb michaeljb left a comment

Choose a reason for hiding this comment

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

I'm sure that when @michaeljb has some time he'll be able to assess the changes that you've made.

@PistonPercy Sorry it took a while to find some time, but this looks good to me! Did a bit of local testing and it's really nice to debug Ruby directly the browser.

Thanks for getting this to work!

@ollybh ollybh merged commit 48b3684 into tobymao:master Oct 7, 2024
1 check passed
@ollybh ollybh mentioned this pull request Oct 24, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core touches shared code not limited to one game or game family
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants