-
Notifications
You must be signed in to change notification settings - Fork 75
Conversation
forgot to mention I also moved swiper to package.json dep |
@Suven can we merge this one in? |
Hey, I wanted to test how this affects ppl who already have the bower-dep installed when updating, but didn't manage to do so. I'll try my best to merge this by tomorrow and release a new version on npm afterwards |
Probably should have submitted this as 2 prs, sorry about that. Got excited cause this was our last bower dep 😂 |
return `if (typeof FastBoot === 'undefined') { ${content} }` | ||
}); | ||
|
||
return new mergeTrees([defaultTree, browserVendorLib]); |
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.
Faced an issue recently that defaultTree
was undefined
and therefore mergeTrees
throws. return browserVendorLib;
might be fine 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 following ember-gestures lead, merging the vendor tree if it is present, would work fine:
treeForVendor: function(defaultTree) {
var trees = [];
var swiperPath = path.join(path.dirname(require.resolve('swiper')), '..');
var browserVendorLib = new Funnel(swiperPath, {
destDir: 'swiper',
include: ['js/swiper.min.js', 'css/swiper.min.css']
});
browserVendorLib = map(browserVendorLib, (content, relativePath) => {
if (relativePath.indexOf('css') !== -1) {
return content;
}
return `if (typeof FastBoot === 'undefined') { ${content} }`
});
if (defaultTree !== undefined) {
trees.push(defaultTree);
}
trees.push(browserVendorLib);
return new mergeTrees(trees);
},
This is the also the approach I used to be Fastboot v1 compatible in my addon ember-cli-g-maps.
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.
Btw this same code is successfully installing the npm dependency on my fork: https://github.com/Matt-Jensen/ember-cli-swiper/tree/latest
Noticed this PR still has a blueprint which adds the original swiper bower dependency after install. @outdoorsy running: |
Any news on this one? I'd be happy to help out if something is blocking this PR. |
Sorry, missed this note. I've just made the change recommended by @Matt-Jensen . TIL, thanks for that! Hopefully that's all that's missing here, I'd love to get back on master. |
I'll merge a few more things tomorrow, do a retest with all new PR's and then ping this PR when the new release is done in npm. Thanks for everybodys input! |
@Suven I see a potential issue with the merged commit d426a4f which I believe relates to #62. We need to be checking if the This is why we do not see any issues during local development, but consumers of this addon experience issues during install. |
Issue was introduced by Ember-Swiper#56.
This PR properly invokes super (it should be bound) and also uses a more robust method to find app (to deal with addons-of-addons situations). The unbounded super call causes an issue in Ember CLI 2.12. pr Ember-Swiper#56 took it out. "Ember Fastboot rc.1 support"
Ember fastboot rc.1 has some breaking changes that need to be handled. More info here:
https://gist.github.com/kratiahuja/d22de0fb1660cf0ef58f07a6bcbf1a1c