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

Implement sourcemap CLI command #530

Merged
merged 6 commits into from
Apr 19, 2022
Merged

Implement sourcemap CLI command #530

merged 6 commits into from
Apr 19, 2022

Conversation

filiptibell
Copy link
Contributor

PR summary

This PR implements a new sourcemap command which produces an instance tree in JSON format in the following shape:

{
  // Equivalent to instance.Name in Roblox
  "name": "...",
  // Equivalent to instance.ClassName in Roblox
  "className": "...",
  // Array of existing file paths that contributed to the creation of this instance
  "filePaths": ["src/path/init.lua", "src/path/init.meta.json"],
  // Array of children with the same structure
  "children": []
}

This enables new & interesting use cases, as well as consistency in other tools/apps when using Rojo.

  • Tools like luau-analyze-rojo and Roblox LSP should no longer need to do their own parsing of file structures and suffixes to get the same instance structure that Rojo provides
  • Stack traces produced in Roblox can now be linked back to their original file paths, potentially enabling much simpler debugging workflows as well as additional features for services such as Sentry

Initial implementation / design details

The command takes a project file as input or defaults to default.project.json just like other CLI commands.

The json data will either be written to a file specified by the --output parameter, similar to the build command, or if omitted will use stdout so that other tools do not have to write to and read a file as a proxy.

A flag --include-non-scripts determines if all instances should be included in the sourcemap, or just scripts. This can greatly reduce sourcemap size if the Rojo project is fully managed. If the flag is not provided, only scripts will be included in the source map. NOTE: Might need a better name or default here?

Copy link
Contributor

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Neat! It would be great if there were some snapshot tests highlighting the output

src/cli/sourcemap.rs Outdated Show resolved Hide resolved
src/cli/sourcemap.rs Outdated Show resolved Hide resolved
filiptibell and others added 3 commits April 8, 2022 22:50
Co-authored-by: JohnnyMorganz <johnnymorganz@outlook.com>
Co-authored-by: JohnnyMorganz <johnnymorganz@outlook.com>
Copy link
Contributor

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and sorry for the delay in getting it reviewed!

I pushed a couple changes onto the branch. Changes fell into a couple categories:

  • Small interface changes: recurse_create_node now accepts a filter function instead of a bool and passes referents instead of instances. This cut down the code a little and made it a little easier to read.
  • Rust-y things: Accepting &Path and &RojoTree, using an empty Vec<T> instead of Option<Vec<T>>. Nothing major here, functionally very similar.
  • Minor formatting

With CI passing, this should be good to go. Great idea and submission!

@LPGhatguy LPGhatguy merged commit 256aba4 into rojo-rbx:master Apr 19, 2022
Dekkonot pushed a commit to UpliftGames/rojo that referenced this pull request Jan 11, 2024
* Initial implementation of sourcemap CLI command

* Update src/cli/sourcemap.rs

Co-authored-by: JohnnyMorganz <johnnymorganz@outlook.com>

* Update src/cli/sourcemap.rs

Co-authored-by: JohnnyMorganz <johnnymorganz@outlook.com>

* Tidy up sourcemap command

* Update CHANGELOG

Co-authored-by: JohnnyMorganz <johnnymorganz@outlook.com>
Co-authored-by: Lucien Greathouse <me@lpghatguy.com>
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.

3 participants