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

Scopes: Update kind to be an arbitrary string in the names field #107

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Conversation

jridgewell
Copy link
Member

This changes kind from an enum of known values, to be an index into the names field holding any arbitrary string. This allows us to better accommodate non-JS languages.

@connor4312
Copy link

Some suggestion of standard name fields may still be useful here for clients, e.g. in types saying 'global' | 'class' | 'function' | 'block' | string, so that they can be presented nicely.

@jridgewell
Copy link
Member Author

I updated to add these as suggested names.

@jkup jkup self-requested a review June 26, 2024 11:11
Copy link
Collaborator

@jkup jkup left a comment

Choose a reason for hiding this comment

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

Latest change looks good to me, I do like having the suggestions there.

@szuend
Copy link
Collaborator

szuend commented Jun 27, 2024

Should the kind offset be relative to the preceding kind? (or absolute for the first start item). This would keep the number small if all scope kinds are grouped somewhere in names together.

@jkup
Copy link
Collaborator

jkup commented Jul 10, 2024

We talked a bit about this during our monthly call. @rbuckton and @szuend are going to take a look and discuss the benefits/cost of relative vs. absolute offsets for these.

@rbuckton
Copy link
Collaborator

rbuckton commented Jul 10, 2024

Absolute vs. relative offsets for kind is only as efficient as the source map generator makes it.

If a generator adds a names entry for a new kind on-demand, it's less likely to be in sequential order, and could theoretically result in an index that requires a 2+ character Base64VLQ encoding.

If a generator is able to preload all names for kind in advance to keep them sequential, but may have already added entries to names (say, due to concatenating multiple inputs into a generated output), the initial kind is only as efficient as the current names count, which could be large enough to require a 2+ character Base64VLQ encoding.

If a generator is able to preload all names for kind in advance to keep them sequential, it may as well do so for all inputs and stuff them at the front of names, then it would only be as efficient as the total number of names entries it needed to add.

In the meeting, I was under the mistaken impression that a relative kind would mutate the counter for names, but that is not the case as the kind offset is tracked independently. After giving this more thought, it does seem like a relative kind makes the most sense. An efficient generator will pack all kind entries at the front of names. With a relative kind, the indices [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16] would be encoded like "ACCCCCCCCCCCCCCCC" (17 characters), while a non-relative kind would be encoded like "ACEGIKMOQSUWYacegB" (18 characters). For fewer than 17 entries there is no difference in the size of the encoded output, but for 17 or more entries, a relative kind can be more efficiently encoded and stored.

@szuend szuend merged commit afb0b37 into main Jul 22, 2024
2 checks passed
@szuend szuend deleted the kind branch July 22, 2024 04:59
github-actions bot added a commit that referenced this pull request Jul 22, 2024
…107)

SHA: afb0b37
Reason: push, by szuend

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
copybara-service bot pushed a commit to ChromeDevTools/devtools-frontend that referenced this pull request Jul 22, 2024
The spec PR tc39/source-map#107 changed the
original scope kind to be an index into the names array given that
'global', 'function', etc. might not be applicable to every authored
language.

This CL aligns the DevTools implementation with the latest spec.

R=pfaffe@chromium.org

Bug: 40277685
Change-Id: Id4b9915aaab6138fbf2dae5eaf48c6e52b999b40
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5724710
Reviewed-by: Philip Pfaffe <pfaffe@chromium.org>
Commit-Queue: Simon Zünd <szuend@chromium.org>
github-actions bot added a commit to nicolo-ribaudo/source-map that referenced this pull request Sep 26, 2024
…c39#107)

SHA: afb0b37
Reason: push, by nicolo-ribaudo

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.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.

6 participants