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

Logger proposal with .d.ts File #3067

Closed
wants to merge 16 commits into from
Closed

Conversation

glenn2223
Copy link
Contributor

@glenn2223 glenn2223 commented May 24, 2021

A proposal for a JS Logging mechanism plus a .d.ts file for shipping with the npm package

Refs: #2979, sass/dart-sass#13

@glenn2223
Copy link
Contributor Author

@nex3, I assume you want the .md file removing (thought I'd ask rather than just removing it)

@glenn2223 glenn2223 changed the title Logger proposal with '.d.ts' File Logger proposal with .d.ts File May 24, 2021
@nex3
Copy link
Contributor

nex3 commented May 25, 2021

I assume you want the .md file removing (thought I'd ask rather than just removing it)

Yes, let's make the .d.ts canonical.

@glenn2223
Copy link
Contributor Author

glenn2223 commented May 25, 2021

I assume you want the .md file removing (thought I'd ask rather than just removing it)

Yes, let's make the .d.ts canonical.

Done

@nex3
Copy link
Contributor

nex3 commented May 25, 2021

FYI, I've added a TypeScript formatter and run it on your .d.ts to make it easier to review.

Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to write out the entire API!

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
export function renderSync(options: Options): Result;

export namespace types {
abstract class SassType {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Dart Sass doesn't export any object with this name, and I don't think Node Sass does either.

Copy link
Contributor Author

@glenn2223 glenn2223 May 25, 2021

Choose a reason for hiding this comment

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

This way just an easy way to add it to the functions parameter - it's not actually exported.

The alternative is types.Null | types.Number | types.String | ....

How would you like to proceed?

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
setA(value: number): void;
}

class List<T extends SassType = SassType> implements SassType {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sass doesn't consider its lists or maps to be typed, so I don't think this API should either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So they should just be List<unknown> and Map<unknown>, is that right?

Commenting/documenting everything
Dropped unnecessary exports
Added exports to types
Various other changes - highlighted by @nex3
@glenn2223
Copy link
Contributor Author

Did you want the interfaces to be types? It's the same but they're more strict (reference)

@nex3
Copy link
Contributor

nex3 commented May 27, 2021

Thanks @glenn2223! I can take it from here, starting with splitting out the TS definition of the existing API into the spec directory so that the logger proposal can go into thelogger directory.

@nex3
Copy link
Contributor

nex3 commented Jun 4, 2021

Closing this out in favor of #3074.

@nex3 nex3 closed this Jun 4, 2021
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