Skip to content

Conversation

@akudev
Copy link
Contributor

@akudev akudev commented Jun 8, 2021

Please fill in this template.

If changing an existing definition:

This PR brings an update for @openui5/types, which is still on a very old outdated version 1.40.

The previous definition files were done by community member @maylukas (thanks!!) who is also the sole owner of the definitions right now, but has stopped working on this project years ago. We contacted him and he confirmed that we could/should take over ownership, hence he is now being removed from the list.

We (@codeworrior, @petermuessig and me) are members of the original OpenUI5 development team, but due to the structure and ways of consumption of this JS library it is not feasible to bundle the declaration files inside, so we would like to continue to use DefinitelyTyped instead.

The definition files need to cover a 10+-years-grown API and are generated, hence not all guidelines can easily be fulfilled and not all common mistakes avoided. We are working on it and on reducing the number of disabled linting rules.
One example is the "no-any-union" rule: while it completely makes sense from pure type-checking perspective, most violations are for cases when a value of type "any" can be either set or bound. For setting, "any" is needed, but for binding, it's still good to have the type "PropertyBindingInfo" present for documentation purposes.
Another example: the "ban-types" rule complains about type "Function": while we could enable and satisfy the rule by saying "(..any) => any|void" instead, this does not actually improve quality/type-safety. Instead, we plan to work on providing the full function signatures in those places (we have to solve an issue in JSDoc for that).

The tests from the previous definition files have been taken over and adapted as closely to the original as possible.

Two links to a "helloworld" project in the README are not working right now, but this project is already finished and about to be published in the next 2-3 days.

@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 8, 2021

@akudev 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

There aren't any other owners of this package, so a DT maintainer will review it.

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

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ A DT maintainer can merge changes when there are no other reviewers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 53680,
  "author": "akudev",
  "headCommitOid": "a1963e29036e9392666531c6a16e5b184759a446",
  "lastPushDate": "2021-06-08T13:20:49.000Z",
  "lastActivityDate": "2021-06-08T20:02:58.000Z",
  "mergeOfferDate": "2021-06-08T19:33:02.000Z",
  "mergeRequestDate": "2021-06-08T20:02:58.000Z",
  "mergeRequestUser": "akudev",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "openui5",
      "kind": "edit",
      "files": [
        {
          "path": "types/openui5/README.md",
          "kind": "markdown"
        },
        {
          "path": "types/openui5/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/jquery.sap.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/openui5-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/openui5/sap.f.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.m.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.tnt.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.codeeditor.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.commons.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.core.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.dt.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.fl.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.integration.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.layout.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.mdc.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.rta.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.suite.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.support.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.table.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.testrecorder.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.unified.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.ui.ux3.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/sap.uxap.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/openui5/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/openui5/tslint.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-linter-tslintjson) and not moving towards it (check: `rules`)"
        }
      ],
      "owners": [],
      "addedOwners": [
        "openui5bot",
        "petermuessig",
        "codeworrior",
        "akudev"
      ],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "amcasey",
      "date": "2021-06-08T19:32:28.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 856700639,
  "ciResult": "pass"
}

@typescript-bot typescript-bot added Edits Owners This PR adds or removes owners No Other Owners This DT module only has one owner, so we can't have someone verify the change. Check Config Changes a module config files labels Jun 8, 2021
@typescript-bot
Copy link
Contributor

🔔 @akudev — 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 added the The CI failed When GH Actions fails label Jun 8, 2021
@typescript-bot
Copy link
Contributor

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

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

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

...because these type definition files cover an entire npm org and do
not match exactly one package
@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Jun 8, 2021
@akudev
Copy link
Contributor Author

akudev commented Jun 8, 2021

I'm a bit puzzled why the "No Other Owners" label and the bot say that I am the only owner when the bot also noted that four owners are added.

@amcasey
Copy link
Contributor

amcasey commented Jun 8, 2021

@akudev The source of truth for ownership is actually https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/.github/CODEOWNERS, which won't be updated until after this PR merges.

Copy link
Contributor

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation! Please keep working on those lint suppressions.

@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Jun 8, 2021
@akudev
Copy link
Contributor Author

akudev commented Jun 8, 2021

Ready to merge

@typescript-bot typescript-bot merged commit 2cc532a into DefinitelyTyped:master Jun 8, 2021
@akudev
Copy link
Contributor Author

akudev commented Jun 8, 2021

@amcasey: thanks for the quick approval!
We'll get rid of some of the lint suppressions, but unfortunately by far not of all.

@typescript-bot
Copy link
Contributor

I just published @types/[email protected] to npm.

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 Edits Owners This PR adds or removes owners Maintainer Approved No Other Owners This DT module only has one owner, so we can't have someone verify the change. Self Merge This PR can now be self-merged by the PR author or an owner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants