adopted-ember-addons / ember-stripe-elements

A simple Ember wrapper for Stripe Elements
https://ember-stripe-elements.netlify.com
MIT License
19 stars 23 forks source link

Upgrade to octane #14

Closed patrick-emmanuel closed 3 years ago

patrick-emmanuel commented 4 years ago

Update library to octane

knownasilya commented 4 years ago

FYI This change will force users to have to use Ember 3.15+

patrick-emmanuel commented 4 years ago

What would you recommend @knownasilya?

knownasilya commented 4 years ago

Well, most people won't upgrade to the new version yet, so that would mean you'd have to maintain two major versions, which might be just fine. Otherwise, classic does still work in both classic and octane apps, so staying classic might be the better option. You could update classic to class syntax, and I think that would give you 3.10 + or so. Really it's up to the people maintaining this addon.

st-h commented 4 years ago

Honestly, I am somewhat confused about what actually is the best way to make the change to octane happen. We all want to use octane where possible, may it be for the dev experience or the performance benefit. However, as soon as we start to make that happen with addons, we seem to rule out quite a lot of users (no one knows how many that would actually be) - unless we start to maintain a classic and an octane version of the addon.

When thinking about it, the big problem seems to be the use of the glimmer component, as native class syntax works with some older versions. So, probably we could just change the component import and maybe a few places where @tracked is used and this would at least ease the issue somewhat?

But the thing that is really disappointing to me is that one could read everywhere that octane is fully interoperable with previous versions and I haven't come across any place where it would have been mentioned that this is not the case for addons.

knownasilya commented 4 years ago

The main issue is that @tracked is not compatible with anything less then 3.16 (or 3.15) so you can use @glimmer/component pretty far back, but as soon as you add tracked you limit your version. So you could upgrade to glimmer components but still use CPs and theoretically that should be as backwards compatible as class syntax.

mattfrankjames commented 4 years ago

Is there a recommendation for using an Octane variant of this? Can I install this branch as an add on? (Somewhat new to Ember).

lindyhopchris commented 4 years ago

@the-bionic thanks for the PR.

@st-h what do you think? Maybe tag the master branch as 1.0.0, then merge this PR and tag as 2.0.0? Or is this too aggressive an upgrade at the momet and we should wait for Octane to bed in more?

@mattfrankjames the current master branch will work with an Ember app that's either classic or Octane.

simonc commented 3 years ago

Ember LTS releases are anchor points the community can expect add-ons to stick to. v3.20 is the current LTS and v3.16 is the previous one. Any bugfix or security support has been dropped for v3.12 on October 1st.

IMHO this means that it's reasonable for add-ons to require Ember v3.16+ from now on.

What do you think? 🧡🐹

st-h commented 3 years ago

I think there won't be much of an issue, if we create a new major release which is not backwards compatible with older ember versions. We have had no need to release a fix or a dependency upgrade (for probably a year now). The worst thing that could happen would be that we discover a critical vulnerability, which we would need to back port, so that people using older versions of ember also could make use of it.

knownasilya commented 3 years ago

Now that it's later and Octane has had a chance to mature, I think making it 3.16 and Glimmer components is totally fine.

BnitoBzh commented 3 years ago

@knownasilya @the-bionic What's the next step for this migration? Only test cases need to be updated (ember-try)?

lindyhopchris commented 3 years ago

@st-h I'd be able to do some work getting this Octane upgrade done - have some open source time in the next few weeks. The thing I can't do is publish a 1.0.0 release to NPM. Is that something you can do?

I'd suggest I create a develop branch that we can use to prepare the next (Octane) release. Then that get's merged with master when we're ready to tag an Octane version (as 2.0 or a pre-release). Going forward it would be useful to have the develop branch in addition to the main branch.

BnitoBzh commented 3 years ago

@lindyhopchris @st-h I made an upgrade from the @the-bionic's proposal. I have no answer from @the-bionic to merge my updates in order to update this pull request ... (my pull request fix all CI issues and upgrade to ember@3.23)

lindyhopchris commented 3 years ago

@BnitoBzh are you able to open a PR from your fork of @the-bionic changes directly to this repo? If not, and we still don't hear back from @the-bionic I'll just have to sort it out separately.

I'm going to be looking at this sometime next week.

BnitoBzh commented 3 years ago

@lindyhopchris done. https://github.com/adopted-ember-addons/ember-stripe-elements/pull/23/

lindyhopchris commented 3 years ago

Closed in favour of #23

I've merged that PR with develop - develop now being the branch for the next release. We need to tag master as 1.0.0 then do a 2.0.0 release from develop.

st-h commented 3 years ago

@lindyhopchris sorry, I am currently swamped with other work. If you don't have permission to do a npm release, I'll be happy to add you. I remember we talked about setting npm up for you, but I can't remember if we actually did it.

lindyhopchris commented 3 years ago

@st-h np, i'm pretty swamped at the mo too, but should be able to sort out tagging this repo. I don't have NPM permissions, so can you add me? same username on NPM

st-h commented 3 years ago

@lindyhopchris I've asked in discord #adopted-ember-addons yesterday, because I don't have permission to add someone.

lindyhopchris commented 3 years ago

@st-h thanks!