-
Notifications
You must be signed in to change notification settings - Fork 45
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
load shepherd.js lazily using ember-auto-import #305
Conversation
alternatively we could also replace node-assets with ember-cli import, however that would require ember-cli@2.15 index.js would then look like:
|
We should use ember-auto-import here. We will have to do manual checks for fastboot, since we can no longer use fastboot-transform. |
I am not sure if I understand the fastboot issue correctly. If we do not include shepherd in the fastboot rendering, why do we need the fastboot transform at all? So far I have tested lazy loading with a fastboot enabled ember app in dev mode and everything seems to work well. The default theme is included within the rehydrated app as well. Can you please give some more context about the fastboot issue? |
@st-h the fastboot-transform just wraps Shepherd and stops it from being included in fastboot. Please see their README for a better explanation https://github.com/kratiahuja/fastboot-transform We have to use fastboot-transform to keep Shepherd from being included in fastboot mode. |
@rwwagner90 unless I am completely off track here, I do not include shepherd by default, but my intention is to lazily load it when necessary. The only thing that should be included is the theme.css file, when the user specifies to use a theme. The only case where this should be an issue is when someone tries to load and start the tour from something like a model hook (without guarding fastboot mode). I think we could also bake this check into the add on, so that it won't do anything when in fastboot mode. But I am not sure if I actually understand the issue you are trying to point me at. |
@st-h I'm not sure how I can explain it further. fastboot-transform is a wrapper to use with Ember addons to stop them from running in fastboot mode. You'll typically want to use it whenever you use browser API like We currently are using it to ensure we do not load Shepherd in fastboot mode. With the removal of the usage of ember-cli-node-assets, in favor of ember-auto-import, we will have to add in manual fastboot checks to ensure we do not load Shepherd in fastboot. I understand you want to lazily load Shepherd, but we do not want to allow it to lazily load in fastboot, as it will blow up with errors. |
@rwwagner90 I think we are pretty much talking about the same thing 😃 I'll make sure that fastboot will not even lazily load shepherd. |
@st-h gotcha. Let me know when you have implemented the ember-auto-import stuff 😄 |
@rwwagner90 That's why I am confused 🤣 The commit already includes the auto-import stuff. It might be missing some guards for edge cases like an ember hook that is executed by fastboot that could load and start the tour. The only thing that does not use auto-import is the css theme, that is included when the user specifies to use a theme.
|
@st-h ah I was confused by your earlier comments. This should work okay, but I think we should move the styles to be imported without ember-cli-node-assets as well. I am okay with ember-cli 2.15+. In addition to that, I think we should use this https://github.com/elwayman02/ember-is-fastboot and do not do any of the |
use ember-cli to import theme css
@rwwagner90 phew, glad we got this sorted out 😄 I guarded all public methods against fastboot mode - this should all be safe now. If this looks good to you, I'll get tests and documentation going. |
addon/services/tour.js
Outdated
text: [get(this, 'messageForUser')] | ||
}); | ||
return; | ||
if (this.fastboot) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would wrap the entire method in if(!this._isFastBoot)
, rather than resolving a promise here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do need a promise here - or we should change the way shepherd is loaded. For instance:
this.tour.addSteps([
{
}
]).then(() => {
this.tour.start();
})
will blow up in fastboot if we do not return a valid promise here. We could reject the promise, but that might require the user to properly handle errors in the app when in fastboot mode.
We could of course add a load()
method and make that behaviour more explicit (and remove the promise from addSteps()
)
addon/services/tour.js
Outdated
@@ -322,6 +346,9 @@ export default Service.extend(Evented, { | |||
* @public | |||
*/ | |||
next() { | |||
if (this.fastboot) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we need to check for fastboot in every method, but I suppose it does not hurt. It seems somewhat redundant though. Is there a way we could ensure everything essentially noops in fastboot without the check in each method, I wonder? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that as well, however I did have any really good idea how to achieve the same effect without guarding everywhere. The question is how safe we want ember-shepherd to be in fastboot mode. For instance should we allow something like this in a ember hook which might get executed in fastboot
tour.setStep();
tour.start();
and then do we allow any other methods in fastboot? like someone wants to show a specific step when the app has loaded, then we would also need to guard methods like show()
Or maybe there is a way to just inject a dummy service in fastboot mode, which essentially does nothing (however I do not know how from the top of my head)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of a dummy service or potentially having a function that would set all the methods to noops, then set them back when not in fastboot.
@st-h overall, this looks great! I left a few comments on a review, but other than that, we should be good to go! |
@st-h I asked around and I think what we would want to do is use a node only initializer to swap in a fastboot compliant shepherd service http://ember-fastboot.com/docs/addon-author-guide#node-only-initializer ember-fetch kind of does something similar. I think that should be a separate, follow up PR to this though. If you could get the tests passing here, and update the docs, I think this should be good to merge, and we can improve upon it with the fastboot specific dummy service after this is merged. |
…and shepherd has not been fully loaded
That sounds like a good idea!
This looks like it should use node 8 instead of 6? I am not sure what that test is and why this is showing up now. |
Ah, I see. It is supposed to ignore the lock file: Interestingly stylelint-scss claims that 3.6.0 should be running with node 6 or newer... 🤔
Should we use node 8 or do you have any other suggestion to fix this? |
@st-h this looks good to me! I assume you have tried it in your app and the lazy loading works? Does it help your app performance? After merging this, would you be interested in helping with the fastboot dummy service? |
@rwwagner90 I can see a decrease in size of ~20kB gzipped in my app and the test app: locally linked ember-shepherd:
latest release:
So far, I have not found any issues when using the locally linked version. One just needs to take the new promise into account when upgrading. As for the dummy service, if I can make up the time I will definitely try to help. |
fixes #304
This is at an early stage, but I wanted give a chance to review the main concept first. For now I return promises both for the
_initialize
as well as theaddSteps
method. Please let me know in case you prefer to use something like async/await instead (I don't have much experience using async/await yet, so not totally sure how that would work out).Currently this allows to start a tour like:
So, pretty much
addSteps
trigger the loading ofshepherd.js
.Regarding the default theme: This is currently bundled the way it was using node-assets. I tried to find a way to lazy load it or at least include it using auto-import, however have not been successful so far. Currently the css theme gets appended to the app (if one is configured) while the shepherd.js lib is lazily loaded. I think this should not be much of an issue, as the css theme is relatively small. But, let me know if I should look further into if we could use auto-import for that.