Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Fix broken type exports #23

Closed
wants to merge 3 commits into from
Closed

Fix broken type exports #23

wants to merge 3 commits into from

Conversation

jason0x43
Copy link
Member

  • Fix some unexported type issues that weren't being indicated by dts-generator. They will be indicitated by the next version of dts-generator.
  • Update Scheduler to allow QueueItems to be scheduled directly -- this will allow subclasses to use custom QueueItems to annotate scheduled callbacks.

References dojo/dom#12

- Fix some unexported type issues that weren't being indicated by
  dts-generator. They will be indicitated by the next version of
  dts-generator.
- Update Scheduler to allow QueueItems to be scheduled directly -- this
  will allow subclasses to use custom QueueItems to annotate scheduled
  callbacks.

References dojo/dom#12
};
}
else {
item = <QueueItem> callback;
Copy link
Member

Choose a reason for hiding this comment

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

There is currently no test for this condition. Please write one.

@jason0x43
Copy link
Member Author

Hmmm...I'm not terribly happy about exposing QueueItem via schedule. There should be an internal method that does what schedule does (including deferring if necessary) but that only accepts QueueItems. The external API should still only accept callbacks. Reworking (and adding tests)...

Rather than change the public API for Scheduler#schedule, move most of
its functionanlity to a protected _schedule method that accepts a
QueueItem and can be overridden by subclasses.

References dojo/dom#12
@kfranqueiro kfranqueiro changed the title Fix broken type exports, update Scheduler Fix broken type exports May 28, 2015
@eheasley eheasley modified the milestone: Milestone 1 May 29, 2015
@kfranqueiro
Copy link
Member

I've merged this to master in 75704bd.

I have to wonder though... is this technically a deficiency in dts-generator? I mean, the things that are exported in this changeset are really internal to the modules in question, and if you were consuming the ts files in this repository directly, I don't think there would be a problem.

IIUC, the problem is that if they're not exported, dts-generator can't pick them up, but still defines other interfaces as inheriting from the unexported ones? Should it be capable of negotiating that itself, or is the limitation due to language services or something?

@kfranqueiro kfranqueiro closed this Jun 3, 2015
@jason0x43
Copy link
Member Author

The problem is that if an exported interface uses unexported interfaces, dts-generator won't include any of them (explicitly exported or not) in its generated file, but will appear to complete successfully.

@kfranqueiro
Copy link
Member

Oh, so it's not that it just exposes "holey" interfaces, it doesn't end up exposing them at all... But is that due to a limitation of its own, or due to not being able to get information from TS language services about them at all?

@kitsonk
Copy link
Member

kitsonk commented Jun 4, 2015

@kfranqueiro the behaviour of dts-generator is issue #3 but also I don't know if we updated to the dts-generator that was merged on the back of SitePen/dts-generator#21 or not.

@jason0x43
Copy link
Member Author

dts-generator adds typings to a typings file using Program.emit from TypeScript's language services. If some are missing, there's nothing dis-generator can directly do about it.

The change in SitePen/dts-generator#21 does something similar to tsc's noEmitOnError flag. When that flag is set, the compiler will check that no diagnostic messages were generated for a particular emit operation, and will block the emit (and fail) if any were. Without the flag, the compiler will still emit code even when diagnostic messages are generated.

@jason0x43 jason0x43 deleted the typing branch June 16, 2015 12:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants