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 support for triple slash directives #39

Merged

Conversation

unstubbable
Copy link
Contributor

@unstubbable unstubbable commented Jan 19, 2018

An array of triple slash directives can be passed to the emit function. The triple slash directives will be printed before everything else.

@RyanCavanaugh Let me know if you think passing them as third argument to dom.emit is not a good idea.

I would have preferred changing the second param of dom.emit to options: EmitOptions = { rootFlags = ContextFlags.None, tripleSlashDirectives = [] }. But that would obviously be a breaking change.

I also considered making TripleSlashDirective a TopLevelDeclaration. First of all, a triple slash directive is not a declaration. Secondly, you would then have to manually stitch the return values of multiple dom.emit calls together. And it wouldn't be guaranteed that a triple slash directive is not followed by a declaration.

Note: This PR also introduces a custom snapshot serializer for strings, to avoid ugly escaped double quotes in the snapshots.

@RyanCavanaugh
Copy link
Owner

I don't like adding a third parameter; eventually you end up with call sites that look like foo(alpha, undefined, undefined, undefined, 5, undefined, 2, undefined, { }).

Option 1: The second parameter to emit could be made ContextFlags | { rootFlags; tripleSlashDirectives } since we can trivially differentiate the enum from the object; this would be my preferred alternative.

Option 2: Representing /// directives as top-level declarations makes some amount of sense, too, even though it's a lie. We could make emit take TopLevelDeclaration[] | TopLevelDeclaration as its first parameter so callers don't have to manually stitch them together; this seems pretty intuitive. I think it's an open question whether dts-dom itself would do the sorting in that scenario.

Option 3: Snap the current version at 1.0 and take the proposed break to emit as 2.0. Since people are using this library in earnest it'd be good to get on the real semver train.

Opinions?

unstubbable added a commit to unstubbable/dts-dom that referenced this pull request Jan 20, 2018
> Option 1: The second parameter to emit could be made
> `ContextFlags | { rootFlags; tripleSlashDirectives }` since we
> can trivially differentiate the enum from the object;
> this would be my preferred alternative.

see RyanCavanaugh#39 (comment)
unstubbable added a commit to unstubbable/dts-dom that referenced this pull request Jan 20, 2018
> Option 1: The second parameter to emit could be made
> `ContextFlags | { rootFlags; tripleSlashDirectives }` since we
> can trivially differentiate the enum from the object;
> this would be my preferred alternative.

see RyanCavanaugh#39 (comment)
unstubbable added a commit to unstubbable/dts-dom that referenced this pull request Jan 20, 2018
> Option 1: The second parameter to emit could be made
> `ContextFlags | { rootFlags; tripleSlashDirectives }` since we
> can trivially differentiate the enum from the object;
> this would be my preferred alternative.

see RyanCavanaugh#39 (comment)
unstubbable added a commit to unstubbable/dts-dom that referenced this pull request Jan 20, 2018
> Option 3: Snap the current version at 1.0 and take the proposed
> break to emit as 2.0. Since people are using this library in
> earnest it'd be good to get on the real semver train.

see RyanCavanaugh#39 (comment)
unstubbable added a commit to unstubbable/dts-dom that referenced this pull request Jan 20, 2018
> Option 2: Representing /// directives as top-level declarations
> makes some amount of sense, too, even though it's a lie. We
> could make emit take TopLevelDeclaration[] | TopLevelDeclaration
> as its first parameter so callers don't have to manually stitch
> them together; this seems pretty intuitive. I think it's an open
> question whether dts-dom itself would do the sorting in that scenario.

see RyanCavanaugh#39 (comment)
@unstubbable
Copy link
Contributor Author

@RyanCavanaugh I implemented all three options since the changes are really small and I figured we could compare it more easily this way.

From a pure code perspective I prefer option 3. But as you said, you would need to publish 1.0 first and then publish this as 2.0.

My second favourite option would be option 1 since it's basically the same thing in a non-breaking implementation.

Option 2 I like the least for the reasons I already mentioned above in the PR description (at least the remaining ones).

It's your call, I guess. 🙂

@RyanCavanaugh
Copy link
Owner

I've published 1.0. Let's bring over option 3 along with a 1.0 -> 2.0 bump.

@RyanCavanaugh
Copy link
Owner

Please also include a tiny sample in the README below the "New functionality" bullet. Thanks!

unstubbable added a commit to unstubbable/dts-dom that referenced this pull request Jan 22, 2018
> Option 3: Snap the current version at 1.0 and take the proposed
> break to emit as 2.0. Since people are using this library in
> earnest it'd be good to get on the real semver train.

see RyanCavanaugh#39 (comment)

BREAKING CHANGE: Changed the second parameter of emit from
ContextFlags to EmitOptions.
unstubbable added a commit to unstubbable/dts-dom that referenced this pull request Jan 22, 2018
> Option 3: Snap the current version at 1.0 and take the proposed
> break to emit as 2.0. Since people are using this library in
> earnest it'd be good to get on the real semver train.

see RyanCavanaugh#39 (comment)

BREAKING CHANGE: Changed the second parameter of emit from
ContextFlags to EmitOptions.
This will be especially helpful for strings that include double
quotes. With this serializer they won't be printed escaped.
An array of triple slash directives can be passed to the emit
function. The triple slash directives will be printed before
everything else.
> Option 3: Snap the current version at 1.0 and take the proposed
> break to emit as 2.0. Since people are using this library in
> earnest it'd be good to get on the real semver train.

see RyanCavanaugh#39 (comment)

BREAKING CHANGE: Changed the second parameter of emit from
ContextFlags to EmitOptions.
@unstubbable
Copy link
Contributor Author

@RyanCavanaugh I merged option 3 and added an example to the README. I hope the example is tiny enough, I wanted it to be a real-world example, not some foo-bar.

@RyanCavanaugh RyanCavanaugh merged commit 2d06658 into RyanCavanaugh:master Jan 22, 2018
@RyanCavanaugh
Copy link
Owner

Published 2.0 🎉

@unstubbable unstubbable deleted the triple-slash-directives branch January 22, 2018 19:02
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