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

Rename mapbox to maplibre in typings file #172

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Rename mapbox to maplibre in typings file #172

merged 1 commit into from
Jun 16, 2021

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Jun 15, 2021

Launch Checklist

This PR is to update the typings definition to reflect the changes in the maplibre rename of mapbox elements. See #27

  • confirm your changes do not include backports from Mapbox projects (unless with compliant license)
  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog' - already in changelog
  • add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>

@github-actions
Copy link
Contributor

Bundle size report:

Size Change: 0 B
Total Size Before: 205 kB
Total Size After: 205 kB

Output file Before After Change
maplibre-gl.js 196 kB 196 kB 0 B
maplibre-gl.css 8.87 kB 8.87 kB 0 B
ℹ️ View Details No major changes

@HarelM
Copy link
Collaborator Author

HarelM commented Jun 15, 2021

Related to a code review comment as part of #30. If #30 is merged there's no real reason for this PR.

@@ -1,4 +1,4 @@
// Type definitions for MapLibre GL JS 1.14.0-rc.1
// Type definitions for MapLibre GL JS 1.14.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to have the version in the comment? This is something we might often step over and forget to update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe not, not sure, it might be redundant now that's it's inside this repo...

Comment on lines +763 to +766
export type ResponseCallback<T> = (error?: Error, data?: T, cacheControl?: string, expires?: string) => void;

export type Cancelable = { cancel: () => void };

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the context of these two?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Missing types in general. I took the full file from #30 and forgot to remove these two... I can remove them if needed...

@wipfli
Copy link
Contributor

wipfli commented Jun 16, 2021

Thanks @HarelM for the clean-up. Looks good to me ✅

@HarelM
Copy link
Collaborator Author

HarelM commented Jun 16, 2021

So, what's the next step in order to merge this in? (and hopefully release a version with this to npm... :-))

@wipfli
Copy link
Contributor

wipfli commented Jun 16, 2021

I guess just click "squash and merge". You have write access to this repo, right?

@HarelM HarelM merged commit 4886a89 into maplibre:main Jun 16, 2021
@HarelM HarelM deleted the update-typings-to-mablibre branch June 16, 2021 12:38
@HarelM
Copy link
Collaborator Author

HarelM commented Jun 16, 2021

Is there a way for me to publish an npm pakcage as well? Some sort of github action? Other CI I can trigger?

@wipfli
Copy link
Contributor

wipfli commented Jun 16, 2021

@wipfli
Copy link
Contributor

wipfli commented Jun 16, 2021

You can go ahead and create a release candidate on NPM by pushing a tag of the form v1.14.1-rc.1. Please try to push such a tag including the -rc.1. Then we will see if you have the rights to do so.

@wipfli
Copy link
Contributor

wipfli commented Jun 16, 2021

And of course let's first publish release candidates and let's wait with actual releases.

@HarelM
Copy link
Collaborator Author

HarelM commented Jun 16, 2021

Sure thing. I'll do it later tonight. I need to commit the pakckage.json file with the version change for rc as well, right?

@wipfli
Copy link
Contributor

wipfli commented Jun 16, 2021

Yes please. NPM will only see the version in the tag, but the docs website will use the version in package.json. In the last release we forgot to change this an this is why we got:

image

@wipfli
Copy link
Contributor

wipfli commented Jun 16, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants