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

Updated "delaunator" typings to version 3.0 #35165

Merged
merged 4 commits into from May 13, 2019

Conversation

Projects
None yet
6 participants
@tobiaskraus
Copy link
Contributor

commented May 2, 2019

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/mapbox/delaunator
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

I updated the JSDoc according the updated README from mapbox/delaunator and its datatypes.
I added Delaunator.coords. As there is no Delaunator.Node anymore (Delaunator.hull changed its datatype), I removed the namespace and exported the class Delaunator directly. So the new import syntax is:

import { Delaunator } from 'delaunator';

(see delaunator-tests.ts) ... which I consider good practice.

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@tobiaskraus Thank you for submitting this PR!

🔔 @DenisCarriere @BTOdell - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@tobiaskraus The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@tobiaskraus

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Today, the last 12 Merge Requests failed, due to the same error.

It seems like it has nothing to do with my changes. Let's wait what happens...

@weswigham weswigham closed this May 4, 2019

Pull Request Status Board automation moved this from Needs Author Attention to Done May 4, 2019

@weswigham weswigham reopened this May 4, 2019

@typescript-bot typescript-bot moved this from Done to Needs Author Attention in Pull Request Status Board May 4, 2019

@typescript-bot typescript-bot moved this from Needs Author Attention to Waiting for Reviewers in Pull Request Status Board May 4, 2019

@BTOdell

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

I looked at the current Delaunator code and it exports the class as default: export default class ...

But these changes are describing it as using a named export. Is this correct? I'm not sure if the DefinitelyTyped build includes running the tests against the actual downloading library (which is needed to answer this question).

// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped

declare class Delaunator<P> {
export class Delaunator<P> {

This comment has been minimized.

Copy link
@DenisCarriere

This comment has been minimized.

Copy link
@tobiaskraus

tobiaskraus May 4, 2019

Author Contributor

Yes, you are right. And when I test it, in an example, my version doesn't work, but export default works.

But when I change to export default and run npm run lint delaunator in DefinitelyTyped, I get this error:

Error: C:/.../DefinitelyTyped/types/delaunator/index.d.ts:8:1
ERROR: 8:1  npm-naming  The types for delaunator specify 'export default' but the source does not mention 'default' anywhere.

whereas with export class, the lint didn't complain.

Do you know why? What should I do? Ignore the lint?
In the meantime I try to dig deeper by myself...

This comment has been minimized.

Copy link
@tobiaskraus

tobiaskraus May 4, 2019

Author Contributor

I see that npm run lint delaunator looks into delaunator.min.js of npms dalaunatator package. (Don't know why, maybe because of package.json "main" attribute) But this file is meant to be required with Node / Browserify. The ES module version lays in delaunator/index.js.

Should I simply try to push it, even though the lint fails, or is there anything I can do to fix this lint error message?

This comment has been minimized.

Copy link
@tobiaskraus

tobiaskraus May 4, 2019

Author Contributor

I tried it anyway (your mentioned changes). But apparently the test breaks, and therefore cannot be merged. The error is the same as I saw locally with npm run lint delaunator:
https://travis-ci.org/DefinitelyTyped/DefinitelyTyped/builds/528247076?utm_source=github_status&utm_medium=notification

I need help here.

This comment has been minimized.

Copy link
@DenisCarriere

DenisCarriere May 5, 2019

Contributor

Would need help from the DefinitelyTyped team on this one

This comment has been minimized.

Copy link
@weswigham

weswigham May 6, 2019

Contributor

(It does return Delaunator; in a UMD pattern, so an export = Delaunator would be appropriate)

This comment has been minimized.

Copy link
@DenisCarriere

DenisCarriere May 6, 2019

Contributor

That's a real tricky one, so that means the Typescript definition would be different for both NodeJS and web?

module resolves

export default class Delaunator {

https://unpkg.com/delaunator@3.0.2/index.js

This comment has been minimized.

Copy link
@DenisCarriere

DenisCarriere May 6, 2019

Contributor

Correct, the UMD pattern export = Delaunator is correct here.

In NodeJS, if you import via CommonJS, here's what is returned.

> require('delaunator')
{ [Function: Delaunator] from: [Function: from] }

This comment has been minimized.

Copy link
@tobiaskraus

tobiaskraus May 6, 2019

Author Contributor

That's a real tricky one, so that means the Typescript definition would be different for both NodeJS and web?

@DenisCarriere yes that's how I understand it now as well.

Another test of mine for Browser (Parcel Bundler):

I tried to change how I understood:

index.d.ts

declare class Delaunator<P> {
  ...
}
export = Delaunator;

delaunator-test.ts

invalid (tsc complains):

import Delaunator from 'delaunator';
// error: Module '".../DefinitelyTyped/types/delaunator/index"' has no default export

import { Delaunator } from 'delaunator';
// error: Module '".../DefinitelyTyped/types/delaunator"' has no exported member 'Delaunator'
// error: This module can only be referenced with ECMAScript imports/exports by turning on the 'esModuleInterop' flag and referencing its default export.

from https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require I see the right way of importing a module which is exported with export = syntax:

import Delaunator = require('delaunator');

Result

  • npm run lint delaunator worked
  • an example project of mine with parcel-bundler:
    • tsc didn't complain
    • but the compiled JavaScript didn't work because Delaunator.from was not a function as Delaunator was an object: { default: class Delaunator }

Two different definition files ?

So yes, parcel-bundler used the ES module version delaunator/index.js version, which requires apparantly a different TypeScript Definition file than Node.js / Browserify needs (for the UMD version)

What to do? I'm lost. It would have been my first Open Source contribution 😞

This comment has been minimized.

Copy link
@weswigham

weswigham May 6, 2019

Contributor

@tobiaskraus use the export = syntax in the definition file and just use the esModuleInterop compiler flag plus the default imports in your project - that should check out both at compile time and runtime.

@@ -1,4 +1,4 @@
import * as Delaunator from 'delaunator';
import { Delaunator } from 'delaunator';

This comment has been minimized.

Copy link
@DenisCarriere

DenisCarriere May 4, 2019

Contributor

Should be

import Delaunator from 'delaunator';

https://github.com/mapbox/delaunator/blob/master/index.js#L5

Also in their tests:

https://github.com/mapbox/delaunator/blob/master/test.js#L3

import { Delaunator } would resolve undefined

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board May 4, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@tobiaskraus One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@tobiaskraus The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@DenisCarriere
Copy link
Contributor

left a comment

👍 looks good to me

Strange that the linting picks up the minified version

delaunator: handle both: UMD & ES module files.
* TypeScript >= 2.7 required

* flag 'esModuleInterop' in tsconfig required

* thanks @weswigham for the help

@typescript-bot typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board May 10, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@sandersn

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@DenisCarriere @BTOdell the 'export default' check is simple right now. It just uses the entrypoint as defined in package.json, which is why it uses the minified code. Then it just looks for the word 'default' in the text, which is way too generous, but catches some errors.

@sandersn sandersn merged commit d58e1fc into DefinitelyTyped:master May 13, 2019

2 checks passed

DefinitelyTyped.DefinitelyTyped Build #20190510.8 succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Pull Request Status Board automation moved this from Check and Merge to Done May 13, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I just published @types/delaunator@3.0.0 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.