[fastboot-fix]: Only define 'fetch' when FastBoot is available

[majew7]

This PR is a fork to fix Issue #69

Don't want this file fastboot-fetch.js to go to browser, when not using FastBoot.

[arjansingh]

@kratiahuja Are @majew7 and I doing it right? 😁

[kratiahuja]

Huh how is this file going to browser? It will be part of the dist but never part of index.html.

[arjansingh]

We're mounting an Ember engine and it's getting included in our engine's dist. No Fastboot involved in this repo either. Potentially is this a build process issue?

On Fri, Sep 8, 2017 at 5:25 PM Krati Ahuja ***@***.***> wrote: Huh how is this file going to browser? It will be part of the dist but never part of index.html. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhksQQxCgG2t9tUi6cI-ABEyXyXbhNMks5sgdsVgaJpZM4PQksf> .

-- Arjan

[kratiahuja]

Something doesn't seem right then. This asset should never be loaded in browser. While the fix will mitigate the issue, I would rather like to understand why it behaving unexpectedly.

[arjansingh]

Ok I'll ping you on slack next week and we can set up some time for a demo or I'll try to repo the problem in a dummy app.

On Sat, Sep 9, 2017 at 10:13 AM Krati Ahuja ***@***.***> wrote: Something doesn't seem right then. This asset should never be loaded in browser. While the fix will mitigate the issue, I would rather like to understand why it behaving unexpectedly. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhksSFBijglRQbS2VwrBJB0C8P5Jxw-ks5sgsdGgaJpZM4PQksf> .

-- Arjan

[kratiahuja]

Sounds good.

[nlfurniss]

@arjansingh a dummy app would be great (or at least post back your convo with krati from Slack) so if this issue comes up again we will better understand the issue and solution!

[arjansingh]

@kratiahuja @nlfurniss arjansingh/ember-bugs#1 That's little branch I did off of my bugs repo that duplicates the bug. Feel free to checkout the branch and use it to debug.

[arjansingh]

Anyone have a chance to debug this? Should we create an issue in the ember-cli-fastboot repo for better tracking?

[kratiahuja]

@arjansingh sorry i haven't had a chance to look at it. I'll take a look at it tomorrow.

[kratiahuja]

@arjansingh @majew7 I looked at the repo. I don't see fastboot-fetch.js being included in the vendor.js or app.js or engine. As mentioned earlier, it will be part of dist since we need it to be dist if an app has fastboot addon installed for it to be used (which is why you see it in the source) but it isn't being used unless you add ember-cli-fastboot addon. You could consider it as an asset that is packaged but never asked for. Therefore, I don't see why you need this check.

I am tempted to close this PR but I will wait for you guys to respond on why you think think this needed.

[arjansingh]

The problem we see is that the fastboot snippits are being sent to the browser and being evaluated if fetch or any other addon like moment is being used. I’ll add some code to the sample app that uses both.

On Fri, Sep 22, 2017 at 9:32 AM Krati Ahuja ***@***.***> wrote: @arjansingh <https://github.com/arjansingh> @majew7 <https://github.com/majew7> I looked at the repo. I don't see fastboot-fetch.js being included in the vendor.js or app.js or engine. As mentioned earlier, it will be part of dist (which is why you see it in the source) but it isn't being used unless you add ember-cli-fastboot addon. You could consider it as an asset that is packaged but never asked for. Therefore, I don't see why you need this check. I am tempted to close this PR but I will wait for you guys to respond on why you think think this needed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhksRW9ItFkOdO7RlWlSyGmRTuZbptpks5sk-ECgaJpZM4PQksf> .

-- Arjan

[kratiahuja]

I am pretty sure that this file will never called in the browser. The reason you are seeing it in the browser is because you are seeing the source of your engine dist.

[kratiahuja]

The file is only going to be eval'd in the fastboot: https://github.com/ember-cli/ember-fetch/blob/master/index.js#L91

[arjansingh]

@kratiahuja I updated arjansingh/ember-bugs#1 to actually call ember-fetch. It most definitely throws an error now:

[kratiahuja]

@arjansingh So I looked at this your repro, the reason fetch is getting replaced by fastboot implementation is because the asset-manifest.json (generated by engines) is asking the browser to go fetch fastboot-fetch.js which it should not. You will see an entry of fastboot-fetch.js in there. There are other fastboot assets (like fastboot-moment) also in the asset-manifest.json. I don't know enough about how this asset-manifest is generated but this looks like an engines bug to me.

A workaround that worked on your repro app is to move ember-fetch from engine's dependency to app's dependency and then I could see the page getting rendered.

I definitely think this is not a ember-fetch bug but a bug in ember engine build. While this PR will resolve your issue but you will end up fetching un required assets for your app which isn't good. I would recommend looking into to see why engine build is adding these assets to asset-manifest.json which is used in the browser.

[arjansingh]

Ok, so should I open an issue on the ember-engines repo for tracking? Or should I open one on ember-cli-fastboot?

On Sun, Oct 1, 2017 at 10:07 PM Krati Ahuja ***@***.***> wrote: @arjansingh <https://github.com/arjansingh> So I looked at this your repro, the reason fetch is getting replaced by fastboot implementation is because the asset-manifest.json (generated by engines) is asking the browser to go fetch fastboot-fetch.js which it *should not*. You will see an entry of fastboot-fetch.js in there. There are other fastboot assets (like fastboot-moment) also in the asset-manifest.json. I don't know enough about how this asset-manifest is generated but this looks like an engines bug to me. A workaround that worked on your repro app is to move ember-fetch from engine's dependency to app's dependency and then I could see the page getting rendered. I definitely think this is not a ember-fetch bug but how engines builds are. While this PR will resolve your issue but you will end up fetching un required assets for your app. I would recommend looking into to see why engine build is adding these assets to asset-manifest.json which is used in the browser. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhksSRIxfSwNuySosD_rRYToXdb3sw7ks5soG90gaJpZM4PQksf> .

-- Arjan

[kratiahuja]

It's an engines issue so you should open it in engines repo.

[kratiahuja]

@arjansingh I am going to close this PR. I think Trent confirmed this is an issue in ember engies. Please feel free to reopen if you disagree.

[arjansingh]

👍 thanks for the help

On Wed, Oct 11, 2017 at 11:14 PM Krati Ahuja ***@***.***> wrote: @arjansingh <https://github.com/arjansingh> I am going to close this PR. I think Trent confirmed this is an issue in ember engies. Please feel free to reopen if you disagree. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhksQEbPbxeoXD0jDElqwB9cukucMTaks5sra44gaJpZM4PQksf> .

-- Arjan