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

Change Ember Object Reopen to using purely the initialize function #184

Closed
wants to merge 1 commit into from

Conversation

tben
Copy link

@tben tben commented Sep 13, 2024

The most recent ember-resolver (v13) update has removed the use of ember objects in the resolver class. So reopen no longer works. This pulse attempts to fix it

@charlesfries

This comment was marked as resolved.

@mkszepp
Copy link
Contributor

mkszepp commented Sep 18, 2024

In generally using private properties of ember is never good option.

I have tested some ways how we can avoid using privates and also without making again a braking change, but unfurtanally there is no clean fix possible without making braking change (also asked shortly to @NullVoxPopuli)
We need to move resolver changes into the app.

Possible fixes for this issue are something like:

import Resolver from 'ember-resolver';
import Application from '@ember/application';
import { withAbilities } from 'ember-can';

class MyApp extends Application {
  Relsolver = withAbilities(Resolver);
}

or

import { Resolver } from 'ember-can';
import Application from '@ember/application';

class MyApp extends Application {
  Relsolver = Resolver;
}

@RobbieTheWagner which fix do you prefer?

@NullVoxPopuli
Copy link

NullVoxPopuli commented Sep 18, 2024

with @embroider/macros you could make a non-breaking change.

this library never declared support for ember-resolver 13, so you can use macros to say, for ember-resolver < 12, do what you've been doing, and for v13+, do the new thing

this would then be a minor release 🎉

@mkszepp
Copy link
Contributor

mkszepp commented Sep 18, 2024

@NullVoxPopuli ember-resolver v13 is already whitelisted...

"ember-resolver": ">= 8.0.0"

Of course we can do something like this, but when ember-cli brings in next time ember-resolver v13, app are running into this issue and they need to look into instructions

@NullVoxPopuli
Copy link

aye, but >= is a optimistic support. It doesn't mean you guarantee all support for all versions.
>= is a library maintainer's friend, because updating every major to include a range is too much work -- it'd be better if things "work by accident", and then if something doesn't issues are reported.

For example, with most addons choosing >= for ember-source, there is not legit expectation (I hope) that there are 0 problems when upgrading ember-source across majors. Just -- if it works great, if not, we report the issue and fix / get a patch release out.

So, in the way of providing a different behavior for ember-resolver 13, since that is a major version bump, I think it makes sense if ember-can has different instructions for that version without a major bump of its own version.
(I hope this makes sense! <3)

@mkszepp
Copy link
Contributor

mkszepp commented Sep 19, 2024

added PR for fixing ember-resolver v13... see #186

@tben tben closed this Sep 20, 2024
@tben tben deleted the patch-1 branch September 20, 2024 09:32
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