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

Add interfaces used by rollup #131

Merged
merged 3 commits into from Mar 12, 2018
Merged

Conversation

guybedford
Copy link
Contributor

This adds missing interfaces used by Rollup in the progress of rollup/rollup#1806.

@guybedford
Copy link
Contributor Author

I've also updated the source map interface types here to make them compatible with the typings from the source-map project.

I added the version: string there because this is necessary for the types to be compatible. It may be worth including this in the code too, but isn't strictly necessary.

@guybedford
Copy link
Contributor Author

Subtle ping

Copy link
Collaborator

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

Thanks! (And sorry for the delay.)

One minor thing that I think could be improved, although I might be misreading and don't feel too strongly.

index.d.ts Outdated
@@ -55,6 +57,8 @@ export interface OverwriteOptions {
}

export default class MagicString {
indentExclusionRanges: Array<ExclusionRange>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks to me like this could also be ExclusionRange | Array<ExclusionRange>, same as elsewhere. (Sneakily, clone uses indentExclusionRanges.slice(), which works with either a single range or an array of ranges.) Could you use the more general type here? (I may also be misreading it, but it looks like MagicString.indent handles either case.)

@guybedford
Copy link
Contributor Author

Thanks @alangpierce for taking a look. I've updated that line - let me know how that seems.

Copy link
Collaborator

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

Thanks!

@alangpierce alangpierce merged commit aa927a2 into Rich-Harris:master Mar 12, 2018
@guybedford
Copy link
Contributor Author

Much appreciated! Any chance of an npm publish soon?

@alangpierce
Copy link
Collaborator

Oops, of course. This is my first time handling a PR in this repo since getting collaborator access, but looks like I have npm access as well, so I should be able to do a release.

@alangpierce
Copy link
Collaborator

Done! Published as v0.22.5.

@guybedford
Copy link
Contributor Author

Great, thank you!

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.

None yet

2 participants