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

Unexport or document mapboxgl.util #1408

Closed
jfirebaugh opened this issue Aug 4, 2015 · 17 comments
Closed

Unexport or document mapboxgl.util #1408

jfirebaugh opened this issue Aug 4, 2015 · 17 comments
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API docs 📜 needs discussion 💬

Comments

@jfirebaugh
Copy link
Contributor

Nothing there is public.

@kelvinabrokwa
Copy link
Contributor

Can we leave it public but undocumented? There are some methods in there that are useful for libraries extending gl-js like in gl-draw.

@tmcw
Copy link
Contributor

tmcw commented Aug 4, 2015

fwiw Leaflet's utils are public and they help leaflet's plugins have a vanilla way to do basic stuff like building 3rd party controls

@jfirebaugh
Copy link
Contributor Author

@kelvinabrokwa gl-js doesn't do "public but undocumented". The choices are either private (and preferably still documented), or public and documented. If there's an undocumented public API, it's a bug.

Committing to something as public API has costs:

  • Public APIs must be held to a higher standard for documentation and tests.
  • Public APIs can't change in backward incompatible ways. They are less malleable, i.e. less useful to gl-js itself, because they can't necessarily change in ways that would be more convenient to gl-js.

gl-js is not a general utility library. Its API should focus on maps, and leave methods which are easily found in myriad third party modules, to those modules.

Something like mbgl.Control is different -- I don't have a problem with exporting that.

@lucaswoj
Copy link
Contributor

lucaswoj commented Aug 5, 2015

One potential compromise is to kill mapboxgl.util and pull in a 3rd party utility library

@mourner
Copy link
Member

mourner commented Aug 7, 2015

Based on Leaflet experience, the costs are not that bad.

Higher standard of docs and tests is beneficial in general regardless of whether it is applied to public APIs or not. I also had no problem breaking compatibility on many util methods and no one ever complained — it's fine as long as it is clearly described in the changelog.

There were also never any attempts to make the util methods more generic — they were always kept focused on Leaflet needs, nevertheless they turned out to be useful for users too. Many plugin devs also depend on them.

It also feels uneasy to use a third-party util library in your app for stuff that's already in the bundle but can't be used because it wasn't exported, thus duplicating code.

@vicapow
Copy link
Contributor

vicapow commented Oct 11, 2015

+1 @lucaswoj's comment. A lot of this could go into a third party lib. What can't could be private. as an ie: https://github.com/component/debounce

@lucaswoj lucaswoj removed the api label Mar 10, 2016
@lucaswoj
Copy link
Contributor

I think it is worth resuming this discussion. I'm still in favor of using a 3rd party utility lib like lodash.

  • in its entirety lodash is only 4kb gzipped
  • we can further reduce the payload size by only requiring the modules we need (e.g. require('lodash/chunk');)
  • great effort as gone into documenting, testing, and optimizing lodash

@mourner
Copy link
Member

mourner commented Mar 10, 2016

I'm 👎 on Lodash, it's bloatware in my opinion and we've been perfectly fine with the current situation, with a minimal set of utils specific to GL JS.

@lucaswoj
Copy link
Contributor

I'm ok with that.

@mourner
Copy link
Member

mourner commented Mar 10, 2016

in its entirety lodash is only 4kb gzipped

To expand on this, in it's entirety it's 21kb gzipped, which is a lot for a few utility methods. 4kb is just the core build. Also, our dependency tree is already heavy, and Lodash is like 4MB alone in node_modules... And if we use separate small modules, it's a dependency hell to manage.

@jfirebaugh
Copy link
Contributor Author

The original question here wasn't "should we use lodash or not", it was "should mapbox-gl-js export general-purpose utility methods".

My opinion is still "no": mapbox-gl-js should export only functionality specific to maps. Exporting general purpose utilities, while perhaps convenient in certain cases, means more work for mapbox-gl-js developers, and exposes internal implementation details we may want to change in the future.

@jfirebaugh jfirebaugh reopened this Mar 10, 2016
@mourner
Copy link
Member

mourner commented Mar 11, 2016

My opinion is still "yes": mapbox-gl-js should leave util exported and document it. Making a method public doesn't mean we are obliged to make it general-purpose — it's enough to document its specifics, which should be documented anyway. If we want to change implementation details in future, it's not a problem — minor breaking changes happen, and they don't cause much trouble if documented.

@bhousel
Copy link
Contributor

bhousel commented Mar 11, 2016

If the utilities are useful for plugin development they should be exported.

@jfirebaugh
Copy link
Contributor Author

But we're looking for ways to structure mapbox-gl-js such that plugins can be written without any formal dependency on mapbox-gl-js, because that would be easier for core developers, plugin developers, and plugin users alike. See #1835 and #2118.

@lucaswoj lucaswoj changed the title unexport mapboxgl.util Unexport or document mapboxgl.util Jul 29, 2016
@jfirebaugh
Copy link
Contributor Author

Examples of util code we couldn't remove if mapbox-gl-js was 1.0 and still exporting util, even though the library itself may no longer use it:

We want the freedom to drop functions like this once we no longer need them. The library should not export util.

@jfirebaugh jfirebaugh added the breaking change ⚠️ Requires a backwards-incompatible change to the API label Oct 22, 2016
@mourner
Copy link
Member

mourner commented Oct 24, 2016

@jfirebaugh I'm sold, let's unexport.

@tmcw
Copy link
Contributor

tmcw commented Oct 24, 2016

Yep, I agree too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API docs 📜 needs discussion 💬
Projects
None yet
Development

No branches or pull requests

7 participants