search

embermap / ember-data-storefront

A collection of APIs that address common data-loading issues.
https://embermap.github.io/ember-data-storefront/
MIT License
138 stars 31 forks source link

Octanify, begin mixin deprecation #474

Open charlesfries opened 3 years ago

charlesfries commented 3 years ago
ryanto commented 3 years ago

Woohoo! Thanks for getting this started!

Before I can review it would be good to get the test suite passing. I know our test suite goes back to 3.12, which might be too old for these changes. If that's the case, feel free to remove any unsupported Ember versions from the CI workflow.

If you remove any versions please also add the minimum supported LTS versions.

Thanks again for taking this on!

ryanto commented 3 years ago

Ok awesome, looks like most tests are passing now.

Ember beta is failing, but it's currently an expected failure with storefront's test suite. If you want you can add ember-beta to the known failures.

It's been a while since someone audited the test suite, so you might have to work through some of these issues to get everything passing. Thanks!

Also, Github is making me "approve" test runs for your PR. I'll see if I can figure out how to get tests to automatically run whenever you push code.

charlesfries commented 3 years ago

No prob, I'll get it done.

charlesfries commented 3 years ago

Finally pushed. I ended up bringing the addon up to 3.28. It still needs to be linted (~85 additional file changes) but I thought I'd hold off blowing up the files changed tab until the PR has been reviewed.

Side note: I'm also seeing this error that seems to be out of our control until liquid-fire is updated.

Error: Assertion Failed: Named outlets were removed in Ember 4.0. See https://deprecations.emberjs.com/v3.x#toc_route-render-template for guidance on alternative APIs for named outlet use cases. ('liquid-fire/templates/components/liquid-outlet.hbs' @ L18:C10)
ryanto commented 3 years ago

Wow thanks again! ❤️

I'm on a big project launch at work this week that should hopefully get a little lighter by end of week.

ryanto commented 2 years ago

Heya Charles! Hope all is well! Are you on Ember discord by chance?

charlesfries commented 2 years ago

@ryanto Yes I am, sent u a message

spruce commented 2 years ago

Is there any movement on this?

jkeen commented 2 years ago

@ryanto Maybe the tests will pass now? ^

jkeen commented 2 years ago

@ryanto psssst, think you can approve this workflow? I think this should work now and would be a huge update for the addon

ryanto commented 2 years ago

Opps, missed your last message. Running now!

jkeen commented 2 years ago

Dang, the tests still didn't pass on 3.12. How far back do you want this addon to support? 3.12 was released in August 2019—in the beforetimes!

ryanto commented 2 years ago

Ha, the beforetimes!

I'm not sure about version because I don't have great insight what Ember versions people that use this addon are on.

I guess 3.28?

charlesfries commented 2 years ago

@ryanto Tangentially related--at some point I would like to remove the mixins in this addon entirely for code clarity purposes (this PR just copy/pastes code from the Store mixin into the new addon/services/store.js file).

In your opinion would it be better to 1) remove the mixins outright and introduce a breaking 1.0.0 version now, or 2) take the ember-simple-auth path and deprecate the mixins while introducing ember-cli-build.js options to opt-out of the mixin initializers (although I'm not sure if I can make the new store.js file opt-in in the same way)