load shepherd.js lazily using ember-auto-import

[st-h]

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 the addSteps 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:

this.tour.addSteps([

   ... steps

]).then(() => {
  // shepherd has been loaded
  this.tour.start();
})

So, pretty much addSteps trigger the loading of shepherd.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.

[st-h]

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:

module.exports = {
  name: require('./package').name,
  included(app) {
    if (app.options && app.options.shepherd && app.options.shepherd.theme) {
      app.import(`node_modules/shepherd.js/dist/css/shepherd-theme-${app.options.shepherd.theme}.css`);
    }
    this._super.included.apply(this, arguments);
  },
  options: {
    babel: {
      plugins: [ require.resolve('ember-auto-import/babel-plugin') ]
    }
  }
};

[RobbieTheWagner]

We should use ember-auto-import here. We will have to do manual checks for fastboot, since we can no longer use fastboot-transform.

[st-h]

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?

[RobbieTheWagner]

@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.

[st-h]

@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.

[RobbieTheWagner]

@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 window etc.

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.

[st-h]

@rwwagner90 I think we are pretty much talking about the same thing 😃 I'll make sure that fastboot will not even lazily load shepherd.
I was initially referring to the css template when asking about auto-import - that probably added some to the confusion.

[RobbieTheWagner]

@st-h gotcha. Let me know when you have implemented the ember-auto-import stuff 😄

[st-h]

@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.
Here we could either:

• use the node-assets approach (as within the current commit)
• use ember-cli (as in my comment above which required at least ember-cli@2.15)
• look into using auto import to lazily load the theme when we load shepherd.js - however I am not sure if this currently is possible at all using auto import. At least my previous attempts failed, but that might just be due to a misconception of the configuration or something related

[RobbieTheWagner]

@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 addSteps or initialize etc stuff if we are in fastboot.

[st-h]

@rwwagner90 phew, glad we got this sorted out 😄
I just had decided to not use that add-on prior to your comment, as the implementation is quite trivial and it relies on a mixin, which afaik are no longer supported when using native class syntax. But I'll happily change that if you want to.

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.

[RobbieTheWagner]

I would wrap the entire method in if(!this._isFastBoot), rather than resolving a promise here.

[st-h]

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())

[RobbieTheWagner]

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? 🤔

[st-h]

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)

[RobbieTheWagner]

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.

[RobbieTheWagner]

@st-h overall, this looks great! I left a few comments on a review, but other than that, we should be good to go!

[RobbieTheWagner]

@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.

[st-h]

That sounds like a good idea!
It looks like the Floating Dependencies test is complaining:

error stylelint-scss@3.6.0: The engine "node" is incompatible with this module. Expected version ">=8". Got "6.17.1"
error Found incompatible module

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.

[st-h]

Ah, I see. It is supposed to ignore the lock file:
yarn install --no-lockfile --non-interactive

Interestingly stylelint-scss claims that 3.6.0 should be running with node 6 or newer... 🤔

Node.js v6 or newer is required. That's because stylelint itself doesn't support Node.js versions below 6.

Should we use node 8 or do you have any other suggestion to fix this?

[RobbieTheWagner]

@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?

[st-h]

@rwwagner90
I have put together a plain new ember app with ember-shepherd and ember-cli-fastboot to test my changes.

I can see a decrease in size of ~20kB gzipped in my app and the test app:

locally linked ember-shepherd:

dist/assets/auto-import-fastboot-0cf5930ef5aca8b6d23a94748dab641b.js: 74.27 KB (21.41 KB gzipped)
dist/assets/vendor-da507610e978c7e3c7e24abc27529d1d.js: 2.61 MB (766.45 KB gzipped)

latest release:

dist/assets/auto-import-fastboot-d41d8cd98f00b204e9800998ecf8427e.js: 0 B
dist/assets/vendor-d49b34e68b78d6aea1de5081d4bed5fe.js: 2.68 MB (786.93 KB gzipped)

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.