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

wicket-leaflet: add module loader support #121

Merged
merged 1 commit into from
Jan 26, 2018
Merged

wicket-leaflet: add module loader support #121

merged 1 commit into from
Jan 26, 2018

Conversation

mpschaeuble
Copy link
Contributor

No description provided.

@arthur-e
Copy link
Owner

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

@mpschaeuble
Copy link
Contributor Author

mpschaeuble commented Jan 23, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

mpschaeuble commented Jan 24, 2018

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
Copy link
Owner

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
Copy link
Contributor

@arthur-e - I think this PR should do the trick: #126

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.

3 participants