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 template interceptor type definition #7928

Merged
merged 3 commits into from
Feb 6, 2016

Conversation

optical
Copy link
Contributor

@optical optical commented Feb 1, 2016

@dt-bot
Copy link
Member

dt-bot commented Feb 1, 2016

rest/rest.d.ts

to author (@Nemo157). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@Nemo157
Copy link
Contributor

Nemo157 commented Feb 1, 2016

Should the type of params be { [key: string]: any }?

@optical
Copy link
Contributor Author

optical commented Feb 1, 2016

Yeah I wasn't certain about that - I think its more correct though, so will change.

Also, with the tests the current ones all have the config type set to any when calling wrap. Should these explicitly use their corresponding config type where appropriate?

    .wrap<defaultRequest.Config>(defaultRequest)
    .wrap<hateoas.Config>(hateoas)
    .wrap<location.Config>(location)

And so forth, or is there a reason not to do this?

It'd be nice if these types could always be correctly inferred, but I can't seem to get to work.

@optical
Copy link
Contributor Author

optical commented Feb 1, 2016

Actually it does seem to infer the types properly most of the time.

With the current incorrect definition it'll infer the config type to be template.Config and will error out because of the extra property.
rest.wrap(template, { params: { extra: 'bang!' } });

However surprisingly this will compile just fine?
rest.wrap<template.Config>(template, { params: { extra: 'bang!' } });

So with the tests I could change them to use inference wherever appropriate, and fill out corresponding stub config too.

Does this sound reasonable?

@Nemo157
Copy link
Contributor

Nemo157 commented Feb 1, 2016

Weird that it works with the explicit type... Changing to use inference everywhere would be good, they may have improved the inference since I first added the config checking.

@Nemo157
Copy link
Contributor

Nemo157 commented Feb 1, 2016

Ah, looks like it broke on TS1.6 for some reason (486828c and #5590). If changing back to inferred types works then that would be good. I do remember having some issues with the inference not always working, but can't remember exactly what they were.

.wrap(xhr)
.wrap(noop)
.wrap(fail)
.wrap<KnownConfig>(knownConfig, { prop: 'value' })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inference doesn't work here

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, it's failing to derive the correct type for knownConfig as it doesn't have an init method in the config it passes to interceptor. There's probably some way to change the definition of rest/interceptor to allow it to correctly derive it.

@Nemo157
Copy link
Contributor

Nemo157 commented Feb 2, 2016

👍

vvakame added a commit that referenced this pull request Feb 6, 2016
Add template interceptor type definition
@vvakame vvakame merged commit 635e8ee into DefinitelyTyped:master Feb 6, 2016
someonewithpc added a commit to someonewithpc/DefinitelyTyped that referenced this pull request Aug 18, 2022
  - Make DivOverlay / Tooltip interactive (DefinitelyTyped#7531, DefinitelyTyped#7532 by @johnd0e)
  - Add openOn, close, toggle functions to DivOverlay (DefinitelyTyped#6639 by @johnd0e)
  - Introduce DomEvent.off(el) to remove all listeners (DefinitelyTyped#7125 by @johnd0e)
  - Allow preventing round-off errors by passing false to Util.formatNum / toGeoJSON (DefinitelyTyped#7100 by @johnd0e)
  - Add autoPanOnFocus to Marker (DefinitelyTyped#8042 by @IvanSanchez)
  - Add referrerPolicy to TileLayer (DefinitelyTyped#7945 by @natevw)
  - Add playsInline to VideoOverlay (DefinitelyTyped#7928 by @Falke-Design)
  - Add getCenter to ImageOverlay (DefinitelyTyped#7848 by @Falke-Design)
  - Fire a tileabort event when a TileLayer load is cancelled (DefinitelyTyped#6786 by @dstndstn)
  - Add crossOrigin to Icon (DefinitelyTyped#7298 by @syedmuhammadabid)
typescript-bot pushed a commit that referenced this pull request Sep 15, 2022
…ds, as of version 1.8 by @someonewithpc

* Leaflet: Update definitions to include missing methods found in 1.8

A declaration was generated with `dts-gen` and compared with the existing
definition, merging them manually to add missing methods

* Leaflet: Explicitly define `Evented.listens` with the corresponding function type and fix linter errors

* Leaflet: Add tests for added methods

* Leaflet: Add missing Mixin export

* Allow L.geoJSON to accept geojson.GeoJsonObject[], fixing #60930

* Temporarily duplicate the implementation of Events and Evented, so tests pass

* Fix toGeoJson(precision?: number | false) argument for all overrides

* Add missing properties, as pointed out by DangerBotOSS

* Add a 'tileabort' event when a tile load is cancelled

See Leaflet/Leaflet#6786

* Make changes reported in Leaflet's Changelog

  - Make DivOverlay / Tooltip interactive (#7531, #7532 by @johnd0e)
  - Add openOn, close, toggle functions to DivOverlay (#6639 by @johnd0e)
  - Introduce DomEvent.off(el) to remove all listeners (#7125 by @johnd0e)
  - Allow preventing round-off errors by passing false to Util.formatNum / toGeoJSON (#7100 by @johnd0e)
  - Add autoPanOnFocus to Marker (#8042 by @IvanSanchez)
  - Add referrerPolicy to TileLayer (#7945 by @natevw)
  - Add playsInline to VideoOverlay (#7928 by @Falke-Design)
  - Add getCenter to ImageOverlay (#7848 by @Falke-Design)
  - Fire a tileabort event when a TileLayer load is cancelled (#6786 by @dstndstn)
  - Add crossOrigin to Icon (#7298 by @syedmuhammadabid)
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

4 participants