Support ember-simple-auth v2.1

[fenichelar]

No description provided.

[fenichelar]

Travis is driving me up a wall! I can't seem to get it to stop using npm version v3.5.4...

[tben]

Change "npm install -g npm@6" to "npm install npm@6" in travis file. It's due to that fact that ember-cli-release requires a really old version of npm. Might be worth moving to "ember-cli-release-tag" which is a more maintained version.

[fenichelar]

@tben Thank you!!

I am going to clean up my commit log (rewrite history) before we try and get this merged.

[fenichelar]

Changelog:

# Changelog

- Add testing for Node.js v8 and v10 in Travis CI
- Add testing for Ember LTS 3.4, 3.8, 3.12, 3.16
- Add support for ember-simple-auth v2.1 and v3
- Remove ember-cli-release
- Update ember-try
- Replace deprecated merge with assign

[fenichelar]

@jpadilla

Do we want to drop Ember 2.16? It is no longer supported. If yes, I think we should remove it before we merge this.

Other than that, thoughts?

[jpadilla]

Do we want to drop Ember 2.16? It is no longer supported. If yes, I think we should remove it before we merge this.

@fenichelar sounds good to me, thanks for working on this! 🎉

[jelhan]

Thanks a lot for working on this. I also need it for one of my projects. But would love to upgrade to [email protected] directly. Added some review comments that would allow me to do that.

Not sure if supporting multiple versions of ember-simple-auth is really necessary at all. But if we do it, we should run the tests against all supported version.

[fenichelar]

@tben and @jelhan thoughts?

@jpadilla can you please review when possible?

[jelhan]

I'm still worried about the tests.

package.json allows three major versions of ember-simple-auth:

{
  "dependencies": {
    "ember-simple-auth": "^1.6.0 || ^2.1.0 || ^3.0.0" 
  }
}

But the tests are only run against the version of ember-simple-auth currently locked down in package-lock.json, which is 2.1.1. The Floating Dependencies test scenario which is included in latest Ember CLI blueprints is not used in this addon. But it wouldn't help a lot as it would be unpredictable, which major version is picked.

Supporting multiple versions of a dependency increases testing complexity a lot. Therefor I tend to avoid it. But if you want to go that path, the tests should also cover all possible combinations. Or at least cover one common combination per scenario. Maybe using ember-simple-auth@v1 with node@v6 as default for all tests and testing additionally these two scenarios:

• ember-simple-auth@v2 with node@v8 and [email protected]
• ember-simple-auth@v3 with node@v10 and [email protected]

[fenichelar]

@jelhan I think writing tests for every version is not practical. My goal here is to just offer one last version with support for all 3 versions of node and ember-simple-auth. Then make a new major version here with more limited support.

I am still debating the majors, options:

1. Two majors, one for node 8 and ember-simple-auth 2 and one for node 10 and ember-simple-auth 3
2. One major, node 10 and ember-simple-auth 3

I'm leaning towards the first but we would again be lacking tests. But tests can be added if needed, dropping support is permanent.

What do you think of that plan?

[jelhan]

I think writing tests for every version is not practical.

It's not that much work. Ember-try provides nearly everything that we need out of the box. Had some time. Please find the pull request adding missing test coverage here: #1

Then make a new major version here with more limited support.

As we now have a version that support all three version of ember-simple-auth and has test coverage I don't see a need for a major release. We already invested all the time needed. I'm still not convinced that spending all that time on this one was a good decision but we should now also earn the benefits. 😄

Only one concern left from my side:

This pull requests drops support for node 6. This is a breaking change and would require a major version. The only reason it does so is adding ember-cli-release-tag. I'm not sure if a change to the internal release process justifies a new major version.

#2 adds node 6 support back and adjusts the test coverage accordingly.

[fenichelar]

@jelhan Travis does not appear to be using Node v10 for the test case

[jelhan]

@jelhan Travis does not appear to be using Node v10 for the test case

Could you elaborate? Tests are passing and it correctly reports to install and use node 10: https://travis-ci.org/jpadilla/ember-simple-auth-token/jobs/658757270#L485-L491

[fenichelar]

@jelhan You are correct. It installs Node v8, prints the version, and then installs Node v10. A little confusing but it does appear to be working.

[jelhan]

This is ready to be merged in @jpadilla. It should not be a breaking change beside for removing tools that help with releasing. But maybe that's okay for now? Or should we try to add ember-cli-release back in?

[fenichelar]

@jelhan @jpadilla I'm going to add a few more tests for completeness. I'll let you know once ready.

[fenichelar]

@jelhan Assuming all of the tests pass, what do you think about everything now?

BTW, I had no idea how powerful ember-try was. Thanks for teaching me something new!!

I think after this we should consider releasing v5 which would drop support for Node v6 and Node v8. I'd like to continue to support all versions of ember and ember-simple-auth as long as practical. Thoughts?

[fenichelar]

@jpadilla thoughts?

[jelhan]

@jelhan Assuming all of the tests pass, what do you think about everything now?

To be honest I'm not sure if we need such a complete matrix to cover all possible combinations. But as this library is not changing that often it should not eat up to much CI resources. So I don't think that will be an actual problem. Would be great if this one could be merged and released.

[fenichelar]

@jelhan I went a little overboard because I was planning on dropping support for Node v6 and Node v8 immediately after this is merged. What do you think about that? Looking for feedback.

[jelhan]

I was planning on dropping support for Node v6 and Node v8 immediately after this is merged.

Makes sense to me. Should also consider dropping support for ember-simple-auth@v1 and ember-simple-auth@v2 at least as soon as it adds any additional maintenance costs.

[fenichelar]

@jelhan

If you add your review, I can merge this.

I was thinking about leaving support for all three version of ember-simple-auth. For me at least, upgrading Node is easy, rarely an issue. Upgrading ember and other ember libraries can be a big project with lots of breaking changes. I'm using Node v10 but still using ember 2.x and ember-simple-auth v2 in a lot of places because of this. I imagine I'm not the only one in this position. Also, so far, supporting all version of ember-simple-auth has been trivial as there have been no breaking changes that affect this library.

[fenichelar]

@jpadilla release-it v9.8.3 supports Node v6. This could be an alternative to ember-cli-release. But the tentative plan is to drop Node v6 and Node v8 in the next release so we may want to just add in ember-cli-release-tag or the latest release-it at that time.

[jelhan]

Looks good to me but I don't have write access to this repo. Need @jpadilla for merging and releasing.

[fenichelar]

@jelhan Ahh, sorry. Yes, we need @jpadilla to review.

Changelog:

# Changelog

- Add testing for Node.js v8 and v10 in Travis CI
- Add testing for Ember LTS 3.4, 3.8, 3.12, 3.16
- Add support for ember-simple-auth v2 and v3
- Remove ember-cli-release
- Update ember-try
- Replace deprecated merge with assign

[jpadilla]

@fenichelar awesome work, thanks! switching over to ember-cli-release-tag doesn't seem like a bad idea.

[jpadilla]

@fenichelar What's your npm username? I can add you as a maintainer there too

[fenichelar]

It was mostly @jelhan!

My NPM username is fenichelar

[jpadilla]

Thank you too @jelhan 🎉

[fenichelar]

@jpadilla I don't have npm publish permissions. Can you please publish this, v4.0.8.

[fenichelar]

@jpadilla Thanks. Working now.