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.

Sign up for GitHub

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

Jump to bottom

[leaflet] Update to leaflet 1.9.1 #62487

Merged
merged 6 commits into from
Oct 15, 2022
Merged

[leaflet] Update to leaflet 1.9.1 #62487

merged 6 commits into from
Oct 15, 2022

Conversation

someonewithpc
Copy link
Contributor

@someonewithpc someonewithpc commented Sep 30, 2022 •
edited
Loading

Please fill in this template.

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: <>
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

@typescript-bot
Copy link
Contributor

typescript-bot commented Sep 30, 2022 •
edited
Loading

@someonewithpc Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

This PR can be merged once it's reviewed by a DT maintainer.

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 without tests

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.

Inactive

This PR has been inactive for 10 days — please merge or say something if there's a problem, otherwise it will be closed!

Diagnostic Information: What the bot saw about this PR 
{
  "type": "info",
  "now": "-",
  "pr_number": 62487,
  "author": "someonewithpc",
  "headCommitOid": "12f4f568f5b02247c5d1eeaa75ffd75a9bb0b17b",
  "mergeBaseOid": "084f03bfe7bac41da8ac10f46988e454c219ad33",
  "lastPushDate": "2022-09-30T19:29:16.000Z",
  "lastActivityDate": "2022-10-15T08:30:38.000Z",
  "mergeOfferDate": "2022-10-04T22:04:20.000Z",
  "mergeRequestDate": "2022-10-15T08:30:38.000Z",
  "mergeRequestUser": "someonewithpc",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "leaflet",
      "kind": "edit",
      "files": [
        {
          "path": "types/leaflet/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "alejo90",
        "atd-schubert",
        "mcauer",
        "ronikar",
        "life777",
        "henrythasler",
        "captain-igloo",
        "someonewithpc"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "rbuckton",
      "date": "2022-10-04T22:03:34.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "jonkoops",
      "date": "2022-10-01T22:15:52.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "Falke-Design",
      "date": "2022-09-30T19:46:30.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1263854060,
  "ciResult": "pass"
}

@typescript-bot typescript-bot added Possibly Edits Infrastructure Too many files, bot didn't see them all Author is Owner The author of this PR is a listed owner of the package. Too Many Files Not all files scanned by the bot! labels Sep 30, 2022
@typescript-bot
Copy link
Contributor

🔔 @someonewithpc — 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...)

@someonewithpc
Copy link
Contributor Author

someonewithpc commented Sep 30, 2022 •
edited
Loading

Leaflet 1.9 Changelog

Breaking Changes

Expose ESM entrypoint with Leaflet global (Leaflet/Leaflet#8329 by @jonkoops). If you're using Leaflet in a ESM setup with plugins that rely on the L global, you have to use the default import for them to work:

-import * as L from 'leaflet';
+import L from 'leaflet';

API changes

Improvements

##` Workflow

Leaflet 1.9.1 Changelog

@Falke-Design
Copy link

Expose ESM entrypoint with Leaflet global (Leaflet/Leaflet#8329).

Will be reverted soon with 1.9.2

@someonewithpc someonewithpc changed the title [leaflet] Update to leaflet 1.9.1 Draft: [leaflet] Update to leaflet 1.9.1 Sep 30, 2022
@someonewithpc
Copy link
Contributor Author

Expose ESM entrypoint with Leaflet global (Leaflet/Leaflet#8329).

Will be reverted soon with 1.9.2

Didn't know, but I don't think this would require any change on our side, if I understand it correctly

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Untested Change This PR does not touch tests and removed Possibly Edits Infrastructure Too many files, bot didn't see them all Too Many Files Not all files scanned by the bot! labels Sep 30, 2022
@typescript-bot
Copy link
Contributor

Hey @someonewithpc,

😒 Your PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Please consider adding tests to cover the change you're making. Including tests allows this PR to be merged by yourself and the owners of this module.

This can potentially save days of time for you!

@typescript-bot
Copy link
Contributor

🔔 @alejo90 @atd-schubert @mcauer @ronikar @life777 @henrythasler @captain-igloo — 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.

Falke-Design
Falke-Design suggested changes Sep 30, 2022
View reviewed changes
types/leaflet/index.d.ts Outdated Show resolved Hide resolved
types/leaflet/index.d.ts Outdated Show resolved Hide resolved
@someonewithpc
Copy link
Contributor Author

All tests pass but produce the following warning:

{"leaflet": [{"fileName":"/home/hugo/projects/DefinitelyTyped/types/leaflet/index.d.ts","ruleName":"npm-naming","message":"The declaration doesn't match the JavaScript module 'leaflet'. Reason:\nThe JavaScript module exports a property named 'PosAnimation', which is missing from the declaration module."},{"fileName":"/home/hugo/projects/DefinitelyTyped/types/leaflet/index.d.ts","ruleName":"npm-naming","message":"The declaration doesn't match the JavaScript module 'leaflet'. Reason:\nThe JavaScript module exports a property named 'geoJson', which is missing from the declaration module."},{"fileName":"/home/hugo/projects/DefinitelyTyped/types/leaflet/index.d.ts","ruleName":"npm-naming","message":"The declaration doesn't match the JavaScript module 'leaflet'. Reason:\nThe JavaScript module exports a property named 'noConflict', which is missing from the declaration module."}]}

But those properties are in the definition. Is this because the definition doesn't define a global L, or something else?

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Sep 30, 2022
@typescript-bot
Copy link
Contributor

@someonewithpc 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!

@someonewithpc
[leaflet] Remove zoom from polylineCenter and polygonCenter
12f4f56 
They no longer take a `zoom`, as of 1.9.1
@Falke-Design
Copy link

All tests pass but produce the following warning:

{"leaflet": [{"fileName":"/home/hugo/projects/DefinitelyTyped/types/leaflet/index.d.ts","ruleName":"npm-naming","message":"The declaration doesn't match the JavaScript module 'leaflet'. Reason:\nThe JavaScript module exports a property named 'PosAnimation', which is missing from the declaration module."},{"fileName":"/home/hugo/projects/DefinitelyTyped/types/leaflet/index.d.ts","ruleName":"npm-naming","message":"The declaration doesn't match the JavaScript module 'leaflet'. Reason:\nThe JavaScript module exports a property named 'geoJson', which is missing from the declaration module."},{"fileName":"/home/hugo/projects/DefinitelyTyped/types/leaflet/index.d.ts","ruleName":"npm-naming","message":"The declaration doesn't match the JavaScript module 'leaflet'. Reason:\nThe JavaScript module exports a property named 'noConflict', which is missing from the declaration module."}]}

But those properties are in the definition. Is this because the definition doesn't define a global L, or something else?

That is the reason why we will release 1.9.2 soon with out the new ESM entrypoint -> Breaks imports

@typescript-bot typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Sep 30, 2022
@Falke-Design
Copy link

We also changed the API of Events.listens Leaflet/Leaflet#8161

@typescript-bot
Copy link
Contributor

@Falke-Design Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@someonewithpc
Copy link
Contributor Author

someonewithpc commented Sep 30, 2022 •
edited
Loading

We also changed the API of Events.listens Leaflet/Leaflet#8161

@Falke-Design I saw that PR, but it didn't seem to require any changes to the declaration. Do please correct me if I'm wrong, but listens is either (type: string, propagate?: boolean) => boolean or (type: string, fn: LeafletEventHandlerFn, context?: any, propagate?: boolean) => boolean (declarations at https://github.com/someonewithpc/DefinitelyTyped/blob/master/types/leaflet/index.d.ts#L496-L502 and https://github.com/someonewithpc/DefinitelyTyped/blob/master/types/leaflet/index.d.ts#L536)

Falke-Design
Falke-Design approved these changes Sep 30, 2022
View reviewed changes
Copy link

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

Yes sorry my fault. I didn't checked the file only your changes

@typescript-bot typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Sep 30, 2022
@someonewithpc someonewithpc changed the title Draft: [leaflet] Update to leaflet 1.9.1 [leaflet] Update to leaflet 1.9.1 Oct 1, 2022
@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Oct 4, 2022
@typescript-bot
Copy link
Contributor

@someonewithpc: Everything looks good here. I am ready to merge this PR (at 12f4f56) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@alejo90, @atd-schubert, @mcauer, @ronikar, @life777, @henrythasler, @captain-igloo: you can do this too.)

@typescript-bot
Copy link
Contributor

Re-ping @someonewithpc / @alejo90, @atd-schubert, @mcauer, @ronikar, @life777, @henrythasler, @captain-igloo:

This PR has been ready to merge for over a week, and I haven't seen any requests to merge it. I will close it on Nov 3rd (in three weeks) if this doesn't happen.

(If there's no reason to avoid merging it, please do so. Otherwise, if it shouldn't be merged or if it needs more time, please close it or turn it into a draft.)

@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Oct 15, 2022
@someonewithpc
Copy link
Contributor Author

Ready to merge

@typescript-bot typescript-bot merged commit 92e7d4c into DefinitelyTyped:master Oct 15, 2022
@prushforth prushforth mentioned this pull request Jul 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Falke-Design Falke-Design Falke-Design approved these changes

@rbuckton rbuckton rbuckton approved these changes

@jonkoops jonkoops jonkoops approved these changes

Assignees
No one assigned
Labels
Author is Owner The author of this PR is a listed owner of the package. Maintainer Approved Other Approved This PR was reviewed and signed-off by a community member. Popular package This PR affects a popular package (as counted by NPM download counts). Self Merge This PR can now be self-merged by the PR author or an owner Unmerged The author did not merge the PR when it was ready. Untested Change This PR does not touch tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@someonewithpc @typescript-bot @Falke-Design @jonkoops @rbuckton