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

[euwa wrapper] Add types #63928

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

mannuelf
Copy link

Creates types for Puzzel Chat EUWA Wrapper client as per:
https://help.puzzel.com/product-documents/user-guide/puzzel-contact-centre/puzzel-administration-portal/services/chat-configuration/euwa-wrapper-interface

Select one of these and delete the others:

If adding a new definition:

  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
  • If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
  • Represents shape of module/library correctly
  • tslint.json should contain { "extends": "@definitelytyped/dtslint/dt.json" }, and no additional rules.
  • tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

@typescript-bot typescript-bot added this to Needs Author Action in New Pull Request Status Board Jan 15, 2023
@mannuelf mannuelf marked this pull request as ready for review September 26, 2023 05:51
@typescript-bot
Copy link
Contributor

typescript-bot commented Sep 26, 2023

@mannuelf Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

This PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • 🕐 Only a DT maintainer can approve changes when there are new packages added

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 63928,
  "author": "mannuelf",
  "headCommitOid": "8397d684976b944ec7e0d05d03d11c56afc3b7b2",
  "mergeBaseOid": "540c106285ef0bc11e9546a03ee0fceb3cb76635",
  "lastPushDate": "2023-01-15T21:46:15.000Z",
  "lastActivityDate": "2023-10-14T12:50:56.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "euwa-wrapper",
      "kind": "add",
      "files": [
        {
          "path": "types/euwa-wrapper/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/euwa-wrapper/test/euwa-wrapper.tests.ts",
          "kind": "test"
        },
        {
          "path": "types/euwa-wrapper/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/euwa-wrapper/tslint.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-linter-tslintjson) (check: `rules`)"
        }
      ],
      "owners": [],
      "addedOwners": [
        "mannuelf"
      ],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "changereq",
      "reviewer": "jakebailey",
      "date": "2023-10-13T03:49:27.000Z"
    }
  ],
  "mainBotCommentID": 1734872228,
  "ciResult": "pass"
}

@typescript-bot typescript-bot added New Definition This PR creates a new definition package. Check Config Changes a module config files Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Sep 26, 2023
@typescript-bot
Copy link
Contributor

🔔 @mannuelf — you're the only owner, but it would still be good if you find someone to review this PR in the next few days, otherwise a maintainer will look at it. (And if you do find someone, maybe even recruit them to be a second owner to make future changes easier...)

@typescript-bot typescript-bot moved this from Needs Author Action to Needs Maintainer Action in New Pull Request Status Board Sep 26, 2023
@typescript-bot
Copy link
Contributor

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @mannuelf.

(Ping @«anyone?».)

* It will open the chat window and show the next view
* @returns ChatState
*/
getState: () => ChatState;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these supposed to be represented as fields or would methods make more sense?

Copy link
Author

Choose a reason for hiding this comment

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

Hi,

Following their example here I kept it as a method

declare interface ApplicationAPI {
    [method: string]: Function
}

I created this for our own use on sats.no in our typescript/react site, and thought it would be nice to share it with one interested.

thanks for the review, my first time doing this, all tips & suggestions welcome :-)

@@ -0,0 +1,39 @@
import EUWA from 'euwa-wrapper';
Copy link
Member

Choose a reason for hiding this comment

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

this seems wrong, there is no 'euwa-wrapper' package in the ecosystem,
I could be wrong here, though. Is this real life-like code sample?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I used their real life example found here under the heading Basic Usage: click here

I used this same type in our website sats.no. Puzzel did not have any types published so I thought to publish the one I made and used in our site.

thanks for reviewing, all tips welcome this is my first time :-)

Copy link
Author

Choose a reason for hiding this comment

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

oh yeah their registry is not on npm :-(

Connecting to the NPM Registry
A .npmrc file should be created either in your project or on user level. Read more about .npmrc on https://docs.npmjs.com/cli/v6/configuring-npm/npmrc

The following lines should be added:

@puzzel:registry=https://puzzel.pkgs.visualstudio.com/public/_packaging/main/npm/registry/
always-auth=true

Copy link
Member

Choose a reason for hiding this comment

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

I've used their instructions to download their package, and it indeed doesn't have types. But, this package is not on npm, not a global script (like jquery, youtube, etc), and not a runtime (node, bun, etc).

Additionally, there doesn't seem to be a license on that page that says whether or not these types can be copied and republished. The package that gets downloaded doesn't include a license at all!

I'm not convinced that this should (or can) be published on DefinitelyTyped. Can the authors be convinced to publish their code on npm with the the types? Or at least publish in a public registry with some sort of license on the types?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @jakebailey for double checking, I will contact them and ask them if they want to do this. In the meantime should I put this PR draft mode again while I discuss with them, or should I close the PR?

Copy link
Member

Choose a reason for hiding this comment

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

Whichever; the bot will probably close this if it's not merged in a few weeks, so a draft may be more representative.

@@ -0,0 +1,39 @@
import EUWA from 'euwa-wrapper';
Copy link
Member

Choose a reason for hiding this comment

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

I've used their instructions to download their package, and it indeed doesn't have types. But, this package is not on npm, not a global script (like jquery, youtube, etc), and not a runtime (node, bun, etc).

Additionally, there doesn't seem to be a license on that page that says whether or not these types can be copied and republished. The package that gets downloaded doesn't include a license at all!

I'm not convinced that this should (or can) be published on DefinitelyTyped. Can the authors be convinced to publish their code on npm with the the types? Or at least publish in a public registry with some sort of license on the types?

@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Oct 13, 2023
@typescript-bot typescript-bot moved this from Needs Maintainer Action to Needs Author Action in New Pull Request Status Board Oct 13, 2023
@typescript-bot
Copy link
Contributor

@mannuelf 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. Thank you!

@mannuelf mannuelf closed this Oct 14, 2023
@mannuelf mannuelf reopened this Oct 14, 2023
@mannuelf mannuelf marked this pull request as draft October 14, 2023 12:51
@DangerBotOSS
Copy link

Formatting

The following files are not formatted:

  1. types/euwa-wrapper/test/euwa-wrapper.tests.ts

Consider running npx dprint fmt on these files to make review easier.

Generated by 🚫 dangerJS against 8397d68

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

After #67332, this PR will need to be updated to include an .npmignore. See https://github.com/DefinitelyTyped/DefinitelyTyped#npmignore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Check Config Changes a module config files New Definition This PR creates a new definition package. Revision needed This PR needs code changes before it can be merged.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants