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

fix breaking changes to css classnames #203

Merged
merged 2 commits into from
Jul 12, 2021

Conversation

p-j
Copy link
Contributor

@p-j p-j commented Jul 11, 2021

Launch Checklist

TL;DR:
This PR reverts the breaking change introduced in v15.1.0 by adopting a dual naming scheme for the CSS Classe name throughout the project.

Addresses concerns voiced in #83 & #185
Closes #199
Resolves visgl/react-map-gl#1513

  • confirm your changes do not include backports from Mapbox projects (unless with compliant license)
  • briefly describe the changes in this PR
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>

This PR is aimed at making maplibre 1.* more backward compatible and making sure that developers working with it and other mapbox/maplibre related tools are not depending on other packages adopting the new naming scheme right now.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2021

Bundle size report:

Size Change: +862 B
Total Size Before: 205 kB
Total Size After: 206 kB

Output file Before After Change
maplibre-gl.js 196 kB 197 kB +268 B
maplibre-gl.css 8.87 kB 9.46 kB +594 B
ℹ️ View Details
Source file Before After Change
src/ui/control/geolocate_control.js 2.23 kB 2.3 kB +69 B
src/ui/control/attribution_control.js 1.28 kB 1.33 kB +49 B
src/ui/popup.js 1.83 kB 1.87 kB +32 B
src/ui/control/fullscreen_control.js 852 B 883 B +31 B
src/ui/control/navigation_control.js 1.69 kB 1.72 kB +28 B
src/ui/map.js 6.26 kB 6.29 kB +27 B
src/ui/control/logo_control.js 592 B 609 B +17 B
src/ui/handler/box_zoom.js 802 B 813 B +11 B
src/ui/anchor.js 204 B 213 B +9 B
src/ui/control/scale_control.js 695 B 704 B +9 B
src/ui/handler/shim/touch_zoom_rotate.js 302 B 311 B +9 B
src/ui/handler/shim/drag_pan.js 233 B 241 B +8 B
src/ui/marker.js 2.82 kB 2.83 kB +8 B

@p-j
Copy link
Contributor Author

p-j commented Jul 12, 2021

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2021

I'm waiting for CI to pass...

@p-j
Copy link
Contributor Author

p-j commented Jul 12, 2021

I've got some things to fix first, I noticed that in some places I've not split the class list

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2021

Sure thing! Once you think the changes are compete ping me and I'll review the entire change.

@p-j
Copy link
Contributor Author

p-j commented Jul 12, 2021

@HarelM seems you need to approve the workflows for the CI to run

@p-j
Copy link
Contributor Author

p-j commented Jul 12, 2021

All good now @HarelM

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2021

Cool, can you help in the preparation of the release of 1.15.2:

  1. Change version in package.json
  2. Add a section in the change log and move the "add" here up to a new version

This way I'll be able to simply merge this and create a release right afterward.

@HarelM HarelM merged commit efad8e0 into maplibre:main Jul 12, 2021
@p-j p-j deleted the fix/mapbox-css-compat branch July 12, 2021 07:55
@acalcutt
Copy link
Contributor

acalcutt commented Jul 12, 2021

Hi Everyone,

First i want to say thanks for all the hard work. I've been following and tweaking things to work for my map at wifidb.net . it is currently on 1.15.1

I was wondering about these css changes. its is my understanding that now these are being reverted and saved until the 2.0 version. would it bepossible to make a 2.0 branch that the breaking changes can start getting put into?, or a 1.x legacy branch for the old code to live on while the master branch moves on?

it seems eventually these breaking changes should be made because the mix of mapbox and maplibre gets confusing. for example, the one addon i use was mapbox inspect and originaly it was kind of a pain to modify becuase some things needed to be replaced with mapboxgl, while others (css) had to remain mapbox. eventually i got it working, but the mix made it harder to migrate. in 1.15.1 it was simpler, because all the mapbox text got changed to maplibregl, so it made a lot more sense when searching and replacing

I will say i understand the complaint, that this breaks plugins... if it wasnt for the namespace change and css change, it would be a drop in replacement. but even with reverting this css change, the namespace is different and still breaks plugins.really if your going to revert css, it only seems half done without reverting the namespace. without that it still isnt reverse compatible. for example, there is thread that talked about this with a (react?) plugin where they ask if the mapbox namespace could be kept (perhaps with two versions or one that accespt both namespaces). if we had something like that it would be nice keeping things compatible...but otherwise compatiblity has ready been broken. if we had a version that kept the mapbox namespace, keeping the css with that version would make sense...and it would be a real drop in replacement. right now just moving the css still leaves it incompatible.

I will note i like the idea of having 2 versions,one in the mapbox namespace and one in maplibregl namespace, because keeping the namespace allow plugins to continue unmodied, which would be nice because changing plugins you dont own doesn't alway end well. i speak from experience with mapbox inspect, which i fixed by modifing a release, but had touble getting it working from their sourcecode. it would be nice if the namespace and css hadn't changed so my plugins continued to work...but at the same time I think it makes sense for the project to move away from mapbox...

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2021

I would say that the first breaking change that will be merged to main will make it 2.x, either the removal of the css or removal of token or something else. It will happen soon I believe.
For a drop in replacement you can always use version 1.13 that was its main goal.
If anyone wants to continue maintaining version 1.x I can create a breach just before merging the breaking change. This person will need to take care of maintaining and releasing of this "legacy" version as I believe most of the maintainers will focus on the latest and greatest, much like I will.

@acalcutt
Copy link
Contributor

Personally I agree that things should move on, and this reversion (1.15.2) is a step backward. I have already moved my code to expect maplibregl, so this new realease only hurts me. i am going to have to wait untill a new branch with the css changes come, because i really dont want to revert my mapbox inspect changes.

My post is more about making a compatibility branch for the people who actually need a more mapbox release, like this release is for.

i did go into the namespace based on what i have read here and experience, but thats more an aside and understanding where changes like this come from.

@acalcutt
Copy link
Contributor

acalcutt commented Jul 12, 2021

For example, if we had branch from the 1.13 release, people with a intrest in keeping a compable release could make fixes or merge non breaking changes.

Then then move master to 2.0, which is where it seems people want the breaking changes and actually do these name and css changes, which is where it seems we probably should be going anyway.

@p-j
Copy link
Contributor Author

p-j commented Jul 13, 2021

@acalcutt regarding the namespace change, there is a simple workaround to make maplibre works with the larger ecosystem: that I've shared here: #83 (comment)

// webpack.config.js
module.export = {
  // ...
  resolve: {
    alias: {
      'mapbox-gl': 'maplibre-gl'
    }
  }
}

or with rollup

// rollup.config.js
import alias from '@rollup/plugin-alias';

module.exports = {
  // ...
  plugins: [
    alias({
      entries: [
        { find: 'mapbox-gl', replacement: 'maplibre-gl' },
      ]
    })
  ]
};

The classname added / toggled by maplibre however are not so easily fixed and only a wider adoption from the plugin maintainers / tool developer can support this.

That's the reasoning behind this PR: making it possible to continue to support the wider ecosystem while preparing the shift to a more maplibre centric one.

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.

CSS renaming in version 1.15.1 breaks dependency compatibility Maplibre v1.15 compatibility
3 participants