-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: Add support for range mappings proposal #77
Conversation
Hi, thank you for the contribution! I think there is a fundamental misunderstanding here: it seems to me that the both if rmi != 0 && (line_index as u32) == rmi - 1 {
range_tokens.push(tokens.len() as u32)
} making sense—if the current index within the line is the number of the range token, we push the current token index to the list of range tokens. But I think the proposal is meant to work differently: there can be many range mappings per line, and the Consider this example: currently, Correctness aside, I'm really excited about this. These range mappings would've saved me a lot of pain in the past. ¹ Or better yet, as @Swatinem says, a bit vector. |
I believe converting between 0- and 1-based indices causes some unnecessary confusion here.
I would say remove the 1-based indexing in (4) and (5) (and arguably (3) as well) and just assume 0-based indices everywhere. |
Just a heads-up—I haven't forgotten about this, I just haven't had the time to look at it. |
/// Range mapping index is invalid | ||
InvalidRangeMappingIndex(data_encoding::DecodeError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a NOTE: this is a breaking change as Error
is not (yet) marked as #[non_exhaustive]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say if we're ok with making a breaking change here, then we should also put the information whether a token is a range token directly on RawToken
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
We have a very limited number of unit tests that just test the en/decoding of the range mappings.
Can we construct a more e2e test that shows how these are actually used?
@Swatinem I added some tests for |
The test now makes it a bit clearer what this proposal is trying to achieve. So if I understand it correctly, new bitvec mapping adds this With a more visual example: /* foo */ if (some_other_code) { return; }
// ^ - original token
// v - output token
if (some_other_code) { return; }
// ^ - lookup position So the offset between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm!
thanks for working on this and implementing all the feedback ❤️
}; | ||
self.tokens.push(raw); | ||
raw | ||
} | ||
|
||
/// Adds a new mapping to the builder. | ||
#[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, clippy is absolutely right to complain here. Trying to review the tests for this on github, I completely lose track of what all these parameters mean (without inlay hints).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I'm not sure how should I fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, me neither :-( I think this is fine this way, just wanted to call it out as a future improvement opportunity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is definitely on the list for an eventual rework. These signatures can't stay like this.
Yeap, exactly. This will be useful for the minifier, which does not have full input source locations. This is the case because no tools emit a perfect source map while transpiling, and the mappings in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you for working on this!
}; | ||
self.tokens.push(raw); | ||
raw | ||
} | ||
|
||
/// Adds a new mapping to the builder. | ||
#[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is definitely on the list for an eventual rework. These signatures can't stay like this.
**Related issue:** - getsentry/rust-sourcemap#77
### Description Update SWC crates. 1. To keep in sync 2. Prepare usage of source map range mappings. getsentry/rust-sourcemap#77 ### Testing Instructions See next.js counterpart. vercel/next.js#63790 Closes PACK-2861
# Turbopack * vercel/turborepo#7409 <!-- hrmny - chore: add parallel rust frontend and remove unused rust dependencies --> * vercel/turborepo#7920 <!-- Tobias Koppers - remove warning when there is no PostCSS config --> * vercel/turborepo#7929 <!-- Tim Neutkens - Remove environment variables page from Turbopack docs --> * vercel/turborepo#7926 <!-- Tim Neutkens - Remove outdated section --> * vercel/turborepo#7925 <!-- Tim Neutkens - Update Turbopack CSS docs --> * vercel/turborepo#7928 <!-- Tim Neutkens - Update Next.js mention in Turbopack docs --> * vercel/turborepo#7856 <!-- Donny/강동윤 - build: Update `swc_core` to `v0.90.29` --> ### What? Update SWC crates. ### Why? 1. To keep in sync 2. Prepare usage of source map range mappings. getsentry/rust-sourcemap#77 ### How? Closes PACK-2860
### Description Update SWC crates. 1. To keep in sync 2. Prepare usage of source map range mappings. getsentry/rust-sourcemap#77 ### Testing Instructions See next.js counterpart. #63790 Closes PACK-2861
### Description Update SWC crates. 1. To keep in sync 2. Prepare usage of source map range mappings. getsentry/rust-sourcemap#77 ### Testing Instructions See next.js counterpart. #63790 Closes PACK-2861
### Description Update SWC crates. 1. To keep in sync 2. Prepare usage of source map range mappings. getsentry/rust-sourcemap#77 ### Testing Instructions See next.js counterpart. #63790 Closes PACK-2861
### Description Update SWC crates. 1. To keep in sync 2. Prepare usage of source map range mappings. getsentry/rust-sourcemap#77 ### Testing Instructions See next.js counterpart. #63790 Closes PACK-2861
See https://github.com/tc39/source-map-rfc/blob/main/proposals/range-mappings.md