Skip to content

feat: ship TypeScript declaration files - #1837

Merged
lukastaegert merged 3 commits into
rollup:masterfrom
alan-agius4:feature/ts-support
Jan 8, 2018
Merged

feat: ship TypeScript declaration files#1837
lukastaegert merged 3 commits into
rollup:masterfrom
alan-agius4:feature/ts-support

Conversation

@alan-agius4

Copy link
Copy Markdown
Contributor

Closes: #1836

Some code changes needed to be done as some interface needs to be exported.

@lukastaegert

Copy link
Copy Markdown
Member

Thanks, this looks like a useful addition! Note however that the Travis-CI failed dramatically. Since appveyor succeeded, I assume this is an OS specific issue we should figure out.

@alan-agius4
alan-agius4 force-pushed the feature/ts-support branch 4 times, most recently from 95252db to 611768a Compare January 5, 2018 19:49
@alan-agius4

Copy link
Copy Markdown
Contributor Author

@lukastaegert all green, issue was some import paths has wrong casing.

@alan-agius4
alan-agius4 force-pushed the feature/ts-support branch 7 times, most recently from dd927de to 80dc054 Compare January 6, 2018 11:05

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good, thanks a lot! I'll merge it so that it is part of the next release

Comment thread src/ast/CallOptions.ts
withNew: boolean,
args?: Expression[],
caller: TaggedTemplateExpression | CallExpression | NewExpression | Property | void
export interface CallCreateOptions {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree this is probably a better name 👍

import Pattern from './Pattern';
import Expression from './Expression';
import Scope from '../Scopes/Scope';
import Scope from '../scopes/Scope';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@lukastaegert
lukastaegert merged commit ce52935 into rollup:master Jan 8, 2018
@alan-agius4
alan-agius4 deleted the feature/ts-support branch January 8, 2018 06:16
@alan-agius4

Copy link
Copy Markdown
Contributor Author

Awesome cheers 😁

@MattiasBuelens

Copy link
Copy Markdown
Contributor

As a TypeScript developer: thank you! 😄

Note that DefinitelyTyped has a manually authored type definition published as @types/rollup. When the next version of Rollup is released, this type package should be removed in favor for Rollup's own types.

@alan-agius4

alan-agius4 commented Jan 9, 2018 via email

Copy link
Copy Markdown
Contributor Author

@guybedford

Copy link
Copy Markdown
Contributor

This PR seems to be outputting a full individual file compile of Rollup into the dist folder, along with typings for each of the unbuilt files.

Surely we should just be publishing one top-level rollup.d.ts here for the outward API?

I'm not sure I follow the use case for having separate typings per development module too, will TypeScript really be able to match up the types from these individual files to the dist/rollup.js file?

@guybedford

Copy link
Copy Markdown
Contributor

It seems the typings run through "dist/typings/src/node-entry" as the main file in Node and these are working ok here.

The only issue is that it doesn't support the import rollup from 'rollup' default form and requires users to write import * as rollup from 'rollup' it would be nice if we could get this fixed too.

@alan-agius4

alan-agius4 commented Jan 9, 2018

Copy link
Copy Markdown
Contributor Author

Well at the moment, rollup doesn't export a default, this has this has nothing to do with typings per see.

Regarding the flatting of .d.ts files this is not yet possible with TypeScript or at least this doesn't come out of the box.

Regarding different types, Typescript doesn't care about environments nor the JS version, there is a single file per module which should contain all types. That's why I choose node-entry as it contains the complete set of types

@MattiasBuelens

Copy link
Copy Markdown
Contributor

Indeed, TypeScript doesn't have a way to output a single declaration file yet, see microsoft/TypeScript#4433.

There are tools that try to do this (such as dts-bundle and dts-generator), but they come with their own limitations. Basically, these tools just concatenate the individual .d.ts files into one file, and fix some naming conflicts along the way. They don't "tree shake" the type definitions, so you still end up with one big file with lots of (internal) modules.

@guybedford

Copy link
Copy Markdown
Contributor

My other worry here is that for example the InputOptions in the API here are just a generic configuration object, so a lot of the type benefits aren't actually there.

It could be worth considering manually maintaining the outward .d.ts API file, especially since the Rollup API is so encapsulated, almost all of the types here are internal.

@alan-agius4

alan-agius4 commented Jan 9, 2018 via email

Copy link
Copy Markdown
Contributor Author

@alan-agius4

alan-agius4 commented Jan 9, 2018

Copy link
Copy Markdown
Contributor Author

Re ‘InputOptions’ it doesn look like a configuration object and is correctly defined with the proper types. In general i shouldn’t be able to assign an invalid value to the InputOptions.

InputOptions is actual one of those critical types to have.

The main point of TypeScript or one of the them is to have as strongly typed as possible even for the consumers, and in reality to use the API it can become so easy that in some cases you don’t even need to check the documentation as it will be self explanatory by the types themselves.

@guybedford

Copy link
Copy Markdown
Contributor

@alan-agius4 I'm referring to the generic options we take in https://github.com/rollup/rollup/blob/master/src/rollup/index.ts#L208.

I didn't know it was possible to mark types as "internal" in TypeScript... how would we go about doing this?

@alan-agius4

alan-agius4 commented Jan 9, 2018 via email

Copy link
Copy Markdown
Contributor Author

@alan-agius4

alan-agius4 commented Jan 9, 2018 via email

Copy link
Copy Markdown
Contributor Author

@lukastaegert

lukastaegert commented Jan 10, 2018

Copy link
Copy Markdown
Member

A dictionary seems to contain uniform information which is not the case here?

Anyway the reason we are using a generic object is because

  • we do not want to litter the configuration object type with deprecated options
  • we want to check for unexpected keys
  • I was not considering others using this type 😜

I think we should actually handle this differently in the future:

  • Use a type that only contains valid, non-deprecated options in our signature
  • Cast this to a generic object when checking for deprecations and unexpected keys

Anyway, I think there is no harm in keeping this as it is for now (though merging might have been a little premature...) and do incremental improvements. But we should not remove the DefinitelyTyped definitions just yet.

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.

4 participants