Update to remove bower dependency

devotox commented 7 years ago

Uses Node assets to retrieve gmap for apps from the package.json
gmap for apps at the moment is pointing at your github branch till you actuallly publish it as an npm module
Ensures Fastboot compatibility and Ember Engines compatability in a way that has been more widely used by Ember Community
Moves to ESLint
Update to Ember 2.14

Fixes https://github.com/Matt-Jensen/ember-cli-g-maps/issues/90

Stopgap solution for https://github.com/Matt-Jensen/ember-cli-g-maps/issues/66

Matt-Jensen commented 7 years ago

There's a lot of code I really like here. I think the tests would pass if you prevent Travis from using node 4.8.3, but that's just in my experience.

Some concerns I do have are:

I don't understand the ramifications of using a Github master branch to host gmaps-for-apps.js, do you know any potential issues with doing this?

While I really like the way you add gmaps-for-apps into the vendor tree, I have concerns about this method:

_ensureThisImport() {
if (!this.import) {
    this._findHost = function findHostShim() {
let current = this;
let app;

Is this the community adopted approach for supporting engines?

devotox commented 7 years ago

Ha yes i was just about to move Travis to 6.10.3 also if you want i can lock the version of github to the latest master commit and that way it does not update till you want it too or i can lock it to a git tag if that would be better. the _ensureThisImport is the way i have seen most people do it especially some of the bigger ones like https://github.com/simplabs/ember-simple-auth/blob/master/index.js this supports both ember engines and infinitely nested addons

devotox commented 7 years ago

I have now locked this down to tag 0.5.15 of gmaps-for-apps which would mean there is full control of it and it will not just update along with master in case master breaks. Also using Node 6 for the tests so fingers crossed it all works out

Matt-Jensen commented 7 years ago

What is the Fastboot incompatibility you're referring to? Recently added support for Fastboot 1.0.0 via: #87

devotox commented 7 years ago

Yes you are right. Initially I was on a old version and then when i pulled down your repo I saw that you had added Fastboot Compatibility so I should remove that from the title. What i did do now though was simplify the index.js file using fastboot-transform which is the suggested way by the maintainers of Ember Fastboot

Matt-Jensen commented 7 years ago

Thanks for clarifying. There's a lot of great work here, but TBH the thing I'm nervous about is replacing a bower dep with a github dep. Even with pointing to a specific github tag, all the automated builds out there would now be pulling from Github, which is not Github's use case.

Maybe this is not an issue and I'm just being paranoid, but I haven't seen this done before. If Github has some sort of request limiting we could be leaving users with a hard to debug build issue, not to mention slower build times.

While bower dependencies do suck, the amount they suck is already known. I think the safer play may be to keep your ember-cli migration, eslint, and engine support while reverting vendor code to use bower while utilizing fastboot-transform.

devotox commented 7 years ago

It actually is a use case for GitHub and many have wondered if there were is a reason for NPM Registry. NPM has first class support for GitHub repos because it is a widely done thing especially for private repositories.

On Sat, Jul 8, 2017, 6:05 PM Matt-Jensen notifications@github.com wrote:

Thanks for clarifying. There's a lot of great work here, but TBH the thing I'm nervous about is replacing a bower dep with a github dep. Even with pointing to a specific github tag, all the automated builds out there would now be pulling from Github, which is not Github's use case.

Maybe this is not an issue and I'm just being paranoid, but I haven't seen this done before. If Github has some sort of request limiting we could be leaving users with a hard to debug build issue, not to mention slower build times.

While bower dependencies do suck, the amount they suck is already known. I think the safer play may be to keep your ember-cli migration, eslint, and engine support while reverting vendor code to use bower while utilizing fastboot-transform.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Matt-Jensen/ember-cli-g-maps/pull/91#issuecomment-313868358, or mute the thread https://github.com/notifications/unsubscribe-auth/AB-BzoagwxHYrYqfIbN59bwb6MV5xQy9ks5sL7begaJpZM4ORyJf .

devotox commented 7 years ago

Another option would be for you to just run npm publish and actually have an NPM package

On Sat, Jul 8, 2017, 6:33 PM Devonte Emokpae devo.tox.89@gmail.com wrote:

It actually is a use case for GitHub and many have wondered if there were is a reason for NPM Registry. NPM has first class support for GitHub repos because it is a widely done thing especially for private repositories.

On Sat, Jul 8, 2017, 6:05 PM Matt-Jensen notifications@github.com wrote:

Thanks for clarifying. There's a lot of great work here, but TBH the thing I'm nervous about is replacing a bower dep with a github dep. Even with pointing to a specific github tag, all the automated builds out there would now be pulling from Github, which is not Github's use case.

Maybe this is not an issue and I'm just being paranoid, but I haven't seen this done before. If Github has some sort of request limiting we could be leaving users with a hard to debug build issue, not to mention slower build times.

While bower dependencies do suck, the amount they suck is already known. I think the safer play may be to keep your ember-cli migration, eslint, and engine support while reverting vendor code to use bower while utilizing fastboot-transform.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Matt-Jensen/ember-cli-g-maps/pull/91#issuecomment-313868358, or mute the thread https://github.com/notifications/unsubscribe-auth/AB-BzoagwxHYrYqfIbN59bwb6MV5xQy9ks5sL7begaJpZM4ORyJf .

Matt-Jensen commented 7 years ago

I can't argue with that. Just published for you.

Matt-Jensen commented 7 years ago

I believe Travis CI is failing not due to test errors but a timeout during the build: https://github.com/emberjs/data/issues/5016

devotox commented 7 years ago

Thanks for taking this over the line @Matt-Jensen I got a bit busy and I come back and see you have cleaned it all up!! 👍

Matt-Jensen commented 7 years ago

No thank you @devotox. You did the hard part.

Matt-Jensen / ember-cli-g-maps

Update to remove bower dependency #91