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

Allow execution on fastboot #110

Closed
wants to merge 2 commits into from
Closed

Conversation

cesarizu
Copy link

@cesarizu cesarizu commented Oct 26, 2016

This PR is related to #74 and #104 but tries to solve the problem from a different perpective:

  • Adds a guard to index.js so that leaflet is not loaded in fastboot
  • Converts all L globals to a new module
  • Provides a dummy implementation of L

Currently, this allows my test app to run under fastboot with no errors.

@cesarizu cesarizu force-pushed the fastboot branch 2 times, most recently from 58c2b34 to 88941c7 Compare October 26, 2016 08:38
@dbouwman
Copy link

@cesarizu My only question / concern is related to having "dummy no-op" implementation of L.{method}. The reason being that our team would be looking at extending / building additional addons for Esri specific layers. ( see https://esri.github.io/esri-leaflet )

But - I guess as long as our addon and components do checks to see if if(L.someFunction){... do stuff with L.someFunction... } then we should be fine.

@cesarizu
Copy link
Author

cesarizu commented Oct 26, 2016

@dbouwman You are right. I've changed it to use __noSuchMethod__ so that it would catch any method. A more complete solution would be a real leaflet replacement/mock lib that can run on node.js and maybe generate an image or something like that.

@dbouwman
Copy link

👍

@dbouwman
Copy link

dbouwman commented Nov 7, 2016

@miguelcobain Any chance we can get this merged?

@miguelcobain
Copy link
Owner

There are some things here that I don't understand.
Maybe @cesarizu can clarify.

Why are we mocking tileLayer and not marker, for example?

@cesarizu
Copy link
Author

cesarizu commented Nov 7, 2016

For all "simple" methods there's the __noSuchMethod__ catch-all magic there. tileLayer is a special case that has s wms function inside. This is why it's the only one declared separately. Does that make it more clear?

@miguelcobain
Copy link
Owner

@cesarizu ah, ok. Makes sense. Thanks.

Looks like __noSuchMethod__ can be removed at any time: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/noSuchMethod

Since all L.{method}s are called in the createLayer hook, shouldn't we be able to conditionally call that hook here, depending if on fastboot or not? https://github.com/miguelcobain/ember-leaflet/blob/master/addon/components/base-layer.js#L25

As a bonus, existing ember-leaflet plugins would also get fastboot support, as long as they only use L inside createLayer like they're supposed to.

Helpers would need to be no-op'ped if on fastboot, though.

@miguelcobain
Copy link
Owner

miguelcobain commented Nov 7, 2016

The initializer L would be solved like in #104 using https://github.com/ronco/fastboot-filter-initializers

@cesarizu cesarizu force-pushed the fastboot branch 2 times, most recently from fa09f5e to 839ee78 Compare November 14, 2016 03:56
@cesarizu
Copy link
Author

@miguelcobain I've updated this PR to use a Proxy object instead of __noSuchMethod__. It seems that it's the right way to do this and it's part of ES6.

About the initializer, with this code it doesn't need to be changed. Do you think that this still needs the fastboot-filter-initializers stuff?

@cesarizu
Copy link
Author

Also about addons, I've been using ember-leaflet-marker-cluster but it has it's own issues that I'm reviewing separately. But right now I'm thinking that addons will have to be modified individually to support fastboot as well.

@miguelcobain
Copy link
Owner

@cesarizu We can't rely on proxies for this behaviour.
It is true that they are a part of ES6. However, there isn't a way to polyfill them to ES5. More info: https://babeljs.io/docs/learn-es2015/#proxies

I've outlined what I think a good strategy could be in this comment: #110 (comment)

Do you see any problems with that rationale?

@cesarizu
Copy link
Author

Node.js supports proxies since version 6.0. This code would only run on fastboot, and fastboot requires node.js 4.2. I've updated the PR so that it works for version < 6 when using --harmony_proxies. I added a note on the README.md file about this too. Do you think that's enough?

My approach was to make the use of L as transparent as possible to the code, regardless of where it's used. Eventually, this proxy could even provide a "simple" implementation that can render an image in place for the map.

@plcarmel
Copy link

I am in the process of updating my website so it can work with fastboot. I tried the @cesarizu:fastboot branch and I just wanted to report that it worked well for me after I moved to Node.js 6. All I had to do was to import the L module. Good job !

Can we expect this to be merged soon ?

@miguelcobain
Copy link
Owner

Closed in favor of #125
Thanks!

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.

4 participants