-
Notifications
You must be signed in to change notification settings - Fork 352
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 lastGeneratedColumn to typing for MappingItem #374
Conversation
3 similar comments
Looks good to me. |
Any updates on this? Is there any reason why it's still pending merge? |
source-map.d.ts
Outdated
@@ -61,6 +61,7 @@ export interface MappingItem { | |||
source: string; | |||
generatedLine: number; | |||
generatedColumn: number; | |||
lastGeneratedColumn?: number; |
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 it should be
lastGeneratedColumn: number | null;
If I understand code correctly the initial value is null then it can receive number values including Infinity
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.
This makes me think it is likely optional:
https://github.com/mozilla/source-map/blob/master/lib/wasm.js#L48-L51
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.
@ericmorand @samccone which do you think is the better typing:
lastGeneratedColumn?: number;
lastGeneratedColumn: number | null;
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.
My interpretation is that lastGeneratedColumn: number | null;
is correct, I agree with @nikolay-borzov
lastGeneratedColumn?: number;
is equivalent to lastGeneratedColumn: number | undefined;
but it is initialized as null
.
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.
Thanks. Happy to update and land.
Can someone merge this and release a patch? It is a blocker for most of us using TypeScript. EDIT: Well, not exactly a blocker, but having to extend MappingItem in our own code isn't exactly pretty: type ValidMappingItem = MappingItem & {
lastGeneratedColumn: number | null
} |
Thanks @ericmorand |
source-map.d.ts
Outdated
@@ -61,6 +61,7 @@ export interface MappingItem { | |||
source: string; | |||
generatedLine: number; | |||
generatedColumn: number; | |||
lastGeneratedColumn?: number; |
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.
This makes me think it is likely optional:
https://github.com/mozilla/source-map/blob/master/lib/wasm.js#L48-L51
This is generated when someone calls `computeColumnSpans` so we mark the property as optional as well.
This is generated when someone calls
computeColumnSpans
so we mark the property as optional as well.