-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
add: import decoded sourcemaps from SourceMapGenerator #5732
add: import decoded sourcemaps from SourceMapGenerator #5732
Conversation
f9b87f6
to
9788885
Compare
this looks pretty good to me. thanks for the fix! |
avoided commit-squash to reduce diffs .. i assume maintainers can sqash commits? |
How much of this code is taken from another project? Can we update this to follow Svelte's snake case naming conventions? Do we think there are likely to be upstream changes we want to manually pull in, and would using different variables make that more difficult? |
sure. only thing we must keep are the property names, like originalLine, originalColumn which one is less horrible? DecodedSourcemap and SourceMapGenerator are types
very unlikely, last release 3 years ago = abandoned |
let's not mix cases 😄 (though I understand the reason it was proposed)
this is the best of the proposed options in my opinion. though strange that it has both
I would have no idea how to read this Can I propose a new option of |
dee-ess-mapp-from-ess-emm-genn .... easy ; )
sourcemaps and strings .. what about |
|
src/compiler/preprocess/index.ts
Outdated
} | ||
function convertMappings() { | ||
// use global variable: generator | ||
let previousGeneratedLine = 1; |
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 think Conduitry was also asking if we could rename all the other variables and functions to use snake_case rather than camelCase
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.
good fight, good night
https://images-cdn.9gag.com/photo/a4ZXnVZ_700b.jpg
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'm not quite sure how to interpret. I guess you are going to bed. Will you rename the variables later?
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've pushed a commit to rename these and to also do some other tidying up and inlining of functions that are just called once. I'm aiming to merge this today and take a look for anything else screaming to be merged and then cut a release. Although this is technically a new feature, I'm thinking of still releasing this as a patch release.
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. thanks for the quick fix! really excited about this new feature!
Co-authored-by: Conduitry <[email protected]>
fix #5722 in a performant way