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

[DNM] Fastbootification #104

Closed
wants to merge 1 commit into from

Conversation

dbouwman
Copy link

@dbouwman dbouwman commented Oct 7, 2016

This is a first swipe at making this addon compatible with fastboot. I'm not confident this is ready to merge yet - details below...

I basically followed the Fastboot Addon Author Guide, and managed to get:

a) ember fastboot --serve-assets to run w/o errors related to L being undefined.

b) A simple example map works once the page is loaded in the browser.

My need was a demo showing Fastboot + Ember Engines, so I can say that this does work, but various Engine related hacks were needed in both the Engine Addon that was consuming this ember-leaflet, and the Ember App hosting the Engine.

Caveats

  • I have not tried this in a simple vanilla ember app with fastboot, but I expect it will work.
  • I have not tried this in a simple ember app without fastboot - again, I expect it will work.

I will check into both cases next week, but wanted to get this up for others to check out over the weekend if they are so inclined.

Copy link
Owner

@miguelcobain miguelcobain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. 👍


//FASTBOOT-TODO: Exposing L here is a problem
L: Ember.computed('window.L', function(){
if(L){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this computed property would help.
As I understand it, this CP returns undefined if L is undefined.
Isn't that exactly the same as doing L: L in the object definition?

Also, depending on window.L doesn't make a difference. Ember will try to look up a window object in the BaseLayer context.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Fastboot server throws because L is undefined... but using a CP seems to work around this. Agreed that it's a little O_o, but I can say it works.

@@ -1,7 +1,7 @@
/* global L */

export function initialize(/* container, application */) {
L.Icon.Default.imagePath = '/assets/images/';
//L.Icon.Default.imagePath = '/assets/images/';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need this, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - I'm just not clear on where to add it.

The issue I ran into with this may be unique to loading ember-leaflet into an engine, and then loading that engine into an Ember App.
As you can see, I moved this initializer into /browser and combined with fastboot-filter-initializers, it should not be executed in node.

This was the case when I added ember-leaflet into the fastboot-ed Ember app, but once I added it to the Engine, I started to get errors on this line from Node.

@miguelcobain
Copy link
Owner

Also, I don't exactly see how this PR is stopping this.L.marker from being called in node.

@dbouwman
Copy link
Author

From what I can tell, all this.L... calls should be changed to use the CP.

@dbouwman
Copy link
Author

Closing in lieu of #110

@dbouwman dbouwman closed this Oct 26, 2016
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.

2 participants