Skip to content
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

Update to remove bower dependency #91

Merged
merged 8 commits into from
Jul 8, 2017
Merged

Conversation

devotox
Copy link
Contributor

@devotox devotox commented Jul 8, 2017

  • 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
#90

Stopgap solution for
#66

@Matt-Jensen
Copy link
Owner

Matt-Jensen commented Jul 8, 2017

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
Copy link
Contributor Author

devotox commented Jul 8, 2017

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
Copy link
Contributor Author

devotox commented Jul 8, 2017

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
Copy link
Owner

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

@devotox
Copy link
Contributor Author

devotox commented Jul 8, 2017

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

@devotox devotox changed the title Update to remove bower dependency + Fastboot compatibility Update to remove bower dependency Jul 8, 2017
@Matt-Jensen
Copy link
Owner

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
Copy link
Contributor Author

devotox commented Jul 8, 2017 via email

@devotox
Copy link
Contributor Author

devotox commented Jul 8, 2017 via email

@Matt-Jensen
Copy link
Owner

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

@Matt-Jensen
Copy link
Owner

I believe Travis CI is failing not due to test errors but a timeout during the build: emberjs/data#5016

@Matt-Jensen Matt-Jensen merged commit 9212407 into Matt-Jensen:master Jul 8, 2017
@devotox
Copy link
Contributor Author

devotox commented Jul 8, 2017

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants