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

Some issues with types #2316

Closed
adschm opened this issue Sep 13, 2021 · 2 comments
Closed

Some issues with types #2316

adschm opened this issue Sep 13, 2021 · 2 comments
Labels
bug type.d Type definition related

Comments

@adschm
Copy link
Contributor

adschm commented Sep 13, 2021

I'm currently using billboard.js with plain JS files, where ESlint is employed to have soft type-checking in my IDE. The billboard.js types are provided via npm.

This has identified a few issues of different (though generally low) severity, which I'd like to present/report and discuss here, hoping this is considered valuable enough.

Note that all of this is just about the types reported, the operation of the code itself is fine.

1. "this" for load.done:

var bbchart = bb.generate(...) # generate some chart
bbchart.load({
   json: somedata,
   done: function() {
      # in here 'this' has wrong type
      dosomething();
   }
});

Inside the done() function, ESlint won't properly detect the type "Chart" for this, but thinks the type is actually the args array from the definition here:
https://github.com/naver/billboard.js/blob/master/types/chart.d.ts#L347

I'm not sure whether this is an ESlint/TS parsing bug or whether one could specify this for the done?: statement in the definition:
https://github.com/naver/billboard.js/blob/master/types/chart.d.ts#L366

2. zoom() only accepts numbers

zoom([start, end]) only has type number[], although string is accepted for timeseries data:
https://github.com/naver/billboard.js/blob/master/types/chart.d.ts#L226

3. export.preserveaspectratio should be optional

export() works perfectly fine without setting preserveAspectRatio, so the type should be optional

https://github.com/naver/billboard.js/blob/master/types/chart.d.ts#L513

4. export() callback might return void

The callback function used with export() may return nothing (void) and will still work; however, the type requires string return value.

https://github.com/naver/billboard.js/blob/master/types/chart.d.ts#L514

@netil netil added type.d Type definition related bug labels Sep 15, 2021
netil added a commit to netil/billboard.js that referenced this issue Sep 15, 2021
Fix wrong type definition for
- .load(), .zoom() callback option
- make prserveAspectRatio option from .export() as optional
- specify Plugin interface context

Ref naver#2316
@netil
Copy link
Member

netil commented Sep 15, 2021

@adschm, thanks for looking into this!

@netil netil closed this as completed in 5f7779b Sep 15, 2021
github-actions bot pushed a commit that referenced this issue Sep 17, 2021
# [3.2.0-next.5](3.2.0-next.4...3.2.0-next.5) (2021-09-17)

### Bug Fixes

* **plugin:** fix textoverlap plugin ([5e486b5](5e486b5)), closes [#1144](#1144)
* **types:** Fix wrong type definition ([5f7779b](5f7779b)), closes [#2316](#2316)
* **zoom:** Fix zoomend call on .zoom()  ([9515565](9515565)), closes [#2217](#2217) [#2254](#2254)
@adschm
Copy link
Contributor Author

adschm commented Sep 23, 2021

I've tested with 3.2.0-next.5, and all issues are resolved there except the zoom for string dates.

I will provide a PR for this in a minute, and also add one for another type issue I found recently.

adschm added a commit to adschm/billboard.js that referenced this issue Sep 23, 2021
The changes in 5f7779b ("fix(types): Fix wrong type definition")
provided support for Date type as argument in zoom(), but string dates
are also accepted and parsed properly, e.g. "2021-01-01T01:02:03.567".

Fix naver#2316

Signed-off-by: Adrian Schmutzler <dev@schmutzler.it>
adschm added a commit to adschm/billboard.js that referenced this issue Sep 23, 2021
The changes in 5f7779b ("fix(types): Fix wrong type definition")
provided support for Date type as argument in zoom(), but string dates
are also accepted and parsed properly, e.g. "2021-01-01T01:02:03.567".

Fix naver#2316

Signed-off-by: Adrian Schmutzler <dev@schmutzler.it>
netil pushed a commit that referenced this issue Sep 27, 2021
The changes in 5f7779b ("fix(types): Fix wrong type definition")
provided support for Date type as argument in zoom(), but string dates
are also accepted and parsed properly, e.g. "2021-01-01T01:02:03.567".

Fix #2316
Close #2333
github-actions bot pushed a commit that referenced this issue Oct 1, 2021
# [3.2.0-next.6](3.2.0-next.5...3.2.0-next.6) (2021-10-01)

### Bug Fixes

* **types:** Allow string dates for zoom() domain ([9cae479](9cae479)), closes [#2316](#2316) [#2333](#2333)
* **types:** Fix type definition for load().json ([25ebb78](25ebb78)), closes [#2334](#2334)

### Features

* **event:** add option for step-before/step-after charts for tooltip to match step behavior ([5f664ba](5f664ba)), closes [#2332](#2332)
github-actions bot pushed a commit that referenced this issue Oct 7, 2021
# [3.2.0](3.1.5...3.2.0) (2021-10-07)

### Bug Fixes

* **axis:** fix y axis stepSize value ([18f6f27](18f6f27)), closes [#2294](#2294)
* **bar:** fix data label to be shown for 0 values ([f3634ee](f3634ee)), closes [#2251](#2251)
* **event:** make consistent tooltip position on step-after ([5d3a5ed](5d3a5ed)), closes [#2287](#2287)
* **gauge:** fix error when interaction=false ([cc4a5e7](cc4a5e7)), closes [#2351](#2351)
* **grid:** Make grid elements pass through pointer events ([4db1bcd](4db1bcd)), closes [#2355](#2355)
* **grid:** pPrevent error throw  ([8fcf61c](8fcf61c)), closes [#2310](#2310)
* **plugin:** fix textoverlap plugin ([5e486b5](5e486b5)), closes [#1144](#1144)
* **subchart:** fix subchart esm import failure ([ba6c2b5](ba6c2b5)), closes [#2255](#2255)
* **tooltip:** fix tooltip.position call context ([b78a48d](b78a48d)), closes [#2265](#2265)
* **types:** Allow string dates for zoom() domain ([9cae479](9cae479)), closes [#2316](#2316) [#2333](#2333)
* **types:** fix data.onshown/hidden types ([3721c4c](3721c4c)), closes [#2270](#2270) [#2275](#2275)
* **types:** Fix type definition for load().json ([25ebb78](25ebb78)), closes [#2334](#2334)
* **types:** Fix wrong type definition ([5f7779b](5f7779b)), closes [#2316](#2316)
* **zoom:** Fix error throw on drag zoom interaction ([f1dcb27](f1dcb27)), closes [#2343](#2343)
* **zoom:** fix zoom event triggering for drag type ([0a0f039](0a0f039)), closes [#2254](#2254)
* **zoom:** Fix zoomend call on .zoom()  ([9515565](9515565)), closes [#2217](#2217) [#2254](#2254)

### Features

* **axis:** Enhance padding to accept px value ([769ec8f](769ec8f)), closes [#2246](#2246)
* **error:** Enhance error logging for ESM import ([4b5119c](4b5119c)), closes [#2311](#2311)
* **event:** add option for step-before/step-after charts for tooltip to match step behavior ([5f664ba](5f664ba)), closes [#2332](#2332)
* **module:** Support dual CJS/ESM package ([ddd8977](ddd8977)), closes [#2202](#2202)
* **plugin:** Intent to ship sparkline ([091284e](091284e)), closes [#2285](#2285)
* **plugin:** Intent to ship TableView plugin ([6f07e94](6f07e94)), closes [#1873](#1873)
* **Subchart:** add subchart x axis tick format option ([da2f3ff](da2f3ff)), closes [#2314](#2314)
* **tooltip:** Enhance tooltip.position passing curr pos ([ec783e9](ec783e9)), closes [#2267](#2267)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug type.d Type definition related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants