arthur-e / Wicket

A modest library for moving between Well-Known Text (WKT) and various framework geometries
https://arthur-e.github.io/Wicket/
Other
586 stars 226 forks source link

wicket-leaflet: add module loader support #121

Closed mpschaeuble closed 6 years ago

arthur-e commented 6 years ago

We have no tests for the Leaflet extension. Can you confirm that the standard WKT test cases work under these changes? Thanks!

mpschaeuble commented 6 years ago

No, doesn't work at all. A simple toObject() fails because an object with the interface {x: number, y: number} is passed to leaflets coordsToLatLng method, which expects a coordinate to be passed a as a two element array. But I'm not sure if this issue is related to this pull request.

I would assume that at least the leaflet part is completely broken. It would probably be best to have a proper automated test setup for each map provider.

arthur-e commented 6 years ago

Maybe I'm not following what's changed in this pull request. The intent with the library-specific extension files:

wicket-leaflet.js
wicket-gmap3.js
wicket-arcgis.js

...Is that they each take care of the translating Wicket's (wicket.js) internal representation of spatial coordinates (which, as you pointed out, are stored as objects, {x: number, y: number}) into the library-specific object. For instance, I believe that the Leaflet extension wicket-leaflet.js does (or did, at one point) transform{x: number, y: number} into a [x, y] array, prior to passing it to the respective Leaflet geometry constructor.

We also do have a specific set of tests for one of the map providers, Google Maps, in the following spec:

tests/wicket-gmap3-spec.js

These are difficult to automate (I currently don't know how) because they require loading external resources in a (headless?) browser. Hence, the Google Maps tests above are not automated. Would love it if someone could find a way to automate them for all providers.

mpschaeuble commented 6 years ago

What the pull request does, is adding the "standard" module loading wrapper as it is already available in wicket.js:

(function ( root, factory ) {
    if ( typeof exports === 'object' ) {
        // CommonJS
        factory( require('./wicket') );
    } else if ( typeof define === 'function' && define.amd ) {
        // AMD. Register as an anonymous module.
        define( ['wicket'], factory);
    } else {
        // Browser globals
        factory(root.Wkt);
    }
}
(this, function(Wkt) {

[...]

}));

I agree this change is not really clear from the github diff page. To my understanding, this change is needed in order to include the two parts (wicket.js and wicket-leaflet.js) in a build using module resolution (in my case it's a angular-cli/webpack build).

Regarding the "coordinate translation" issue: I think currently the wicket-internal coordinate representation is just passed to leaflet. When looking at the code, I think the idea was to override leaflet's "coordsToLatLng" method from wicket-leaflet.js, but this seems not to work (anymore?).

arthur-e commented 6 years ago

Yes, I believe coordsToLatLng() is based on the previous major version of Leaflet and things have changed a lot. See #103 #104 and #119. Hopefully someone will issue a Pull Request with a patch for these issue(s).

nickescallon commented 6 years ago

@arthur-e - I think this PR should do the trick: https://github.com/arthur-e/Wicket/pull/126