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

Refactor into smaller modules #154

Closed
louh opened this issue Mar 10, 2017 · 4 comments
Closed

Refactor into smaller modules #154

louh opened this issue Mar 10, 2017 · 4 comments
Assignees
Milestone

Comments

@louh
Copy link
Contributor

louh commented Mar 10, 2017

  1. Allow importing of a core module without namespacing to the Leaflet object immediately. (see https://github.com/perliedman/leaflet-routing-machine)
  2. Split out XHR/XDR into a separate module or import a dependency (e.g. https://github.com/mapbox/corslite)

Goal: enables cleaner import by mapzen.js.

@louh louh added this to the 1.9.0 milestone Mar 10, 2017
@louh louh self-assigned this Mar 10, 2017
@louh
Copy link
Contributor Author

louh commented Mar 14, 2017

Work is ongoing in the bundle branch: https://github.com/mapzen/leaflet-geocoder/tree/bundle

This restructures the existing single JavaScript file into a number of smaller modules. The bulk of the geocoder logic is in core.js, exported as Geocoder. Utility functions, instead of being baked into the single file, are extracted out. Where possible, core depend on existing third-party libraries:

  • console-polyfill - This polyfills the console and its methods, so that console.log() and console.warn() will not cause the script to crash in IE8. (In IE8 and earlier, the console object does not exist unless developer tools are open.)
  • lodash/throttle and lodash/escapeRegExp - We use the utility functions provided by Lodash instead of maintaining our own. Individual Lodash components can be imported, rather than importing all of Lodash. Note that Lodash components may depend on itself for other components.
    • NOTE: Lodash v4 does not support IE8, but we need to verify whether this is true for the modules we are including. It's possible we may need to back out of Lodash to prevent a semver-breaking browser support change. (or put in es5-shim.)
  • (TBD) - the original AJAX object is a separate module, but replacing it with a third-party library is still a work in progress.

By depending on third-party libraries, we do observe a sizable increase in library file size, from about 20kb minified (6kb minified+gzipped) to about 30kb minified (8kb minified+gzipped). Typically, a 50% increase in library size should be cause for concern. However, we are making this decision trading off against other gains:

  • Depending on well-tested and used third-party options reduces the maintenance burden of some components of the plugin.
  • In larger applications (including mapzen.js) shared components will not be imported more than once, so the overall footprint in bundles can decreased even as an application grows in size.

The entry point for the library is now index.js, which imports Geocoder from core. The sole purpose of index.js is to serve as the root file for the bundler and will attach Geocoder to L.Control.Geocoder. As an improvement to original functionality, it is now also possible to import Geocoder directly from the library:

// v1.8.0 or earlier
// L.Control.Geocoder is created as a side-effect of require()
require('leaflet-geocoder-mapzen')
// ...
var geocoder = new L.Control.Geocoder()

// v1.9.0 (and future)
// The require() value can now be set to a variable.
var MyGeocoder = require('leaflet-geocoder-mapzen')
// ...
var geocoder = new MyGeocoder()
// Note that the side-effect of attaching to L.Control.Geocoder still occurs, so as to not break existing code.

If you want to completely prevent side effects, (e.g. for mapzen.js), your import code should import core directly:

// Requiring only the base container (no L namespace)
var Geocoder = require('leaflet-geocoder-mapzen/src/core')
// or in ES2015
import Geocoder from 'leaflet-geocoder-mapzen/src/core'
// ...
const geocoder = new Geocoder()

const nope = new L.Control.Geocoder() // will be undefined

Note that the main entry point in package.json no longer points at the packaged distribution bundle but to the src/index.js file. This allows application bundlers to share code and strip out redundant dependencies.

In keeping with semantic versioning, I would like to make sure this does not break any existing uses. Because the code architecture has changed, we must do a little bit of ground work to make sure this is the case. (If code breaks and it turns out to be insurmountable, this will be a v2 release, and not v1.9.0.). To test this, please install this branch as a Node module:

npm install git://github.com/mapzen/leaflet-geocoder.git#bundle

Be sure to test this out first without updating your existing code, and then update your code if you wish to try the new import/require features.

@hanbyul-here @rfriberg

@louh
Copy link
Contributor Author

louh commented Mar 16, 2017

Update: to preserve IE8 functionality we are importing the previous utility functions instead of Lodash. (note: we can put back Lodash and drop IE8 compatibility in v2.)

@louh
Copy link
Contributor Author

louh commented Apr 3, 2017

Update: we can incorporate corslite instead of the original AJAX module. This adds about an additional 2kb size increase (un-gzipped) but I think it is worth the effort.

@louh
Copy link
Contributor Author

louh commented Apr 6, 2017

1.9.0 released in 8ae16b1.

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

No branches or pull requests

1 participant