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

Octanify, begin mixin deprecation #474

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

charlesfries
Copy link

@charlesfries charlesfries commented Aug 1, 2021

  • Run npx @ember/octanify
  • Convert <AssertMustPreload /> to Glimmer component (introduces component arg breaking change)
  • Run ember g service store to override default StoreService, so we can get rid of mixins
  • Bump to v3.28
  • Fix tests
  • Convert to native classes

@ryanto
Copy link
Member

ryanto commented Aug 2, 2021

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

ryanto commented Aug 3, 2021

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

No prob, I'll get it done.

@charlesfries
Copy link
Author

charlesfries commented Sep 7, 2021

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

ryanto commented Sep 12, 2021

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

ryanto commented Feb 15, 2022

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

@charlesfries
Copy link
Author

@ryanto Yes I am, sent u a message

@spruce
Copy link

spruce commented Jul 3, 2022

Is there any movement on this?

Lint fixes, updates from main brain
@jkeen
Copy link

jkeen commented Oct 12, 2022

@ryanto Maybe the tests will pass now? ^

@jkeen
Copy link

jkeen commented Nov 7, 2022

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

@ryanto
Copy link
Member

ryanto commented Nov 7, 2022

Opps, missed your last message. Running now!

@jkeen
Copy link

jkeen commented Nov 7, 2022

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

ryanto commented Nov 7, 2022

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

@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)

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

Successfully merging this pull request may close these issues.

4 participants