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

Don't export private normalize.ts function & refactor tests #171

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

mkszepp
Copy link
Contributor

@mkszepp mkszepp commented Jun 4, 2024

This should be the last changes what we need to do for getting ready next release.

With this changes we are shipping the normalize.ts private and peoples which were using public, are getting the error.

The test-app has used "public" this functions. As its not anymore possible it was necessary to use the service to make tests compatible with this last changes.

As this function was always declared as private, its no braking change

@mkszepp
Copy link
Contributor Author

mkszepp commented Jun 4, 2024

@RobbieTheWagner i thinks this is now the last part, what we have needed. From my side, i think that we are now ready for next release

@RobbieTheWagner RobbieTheWagner merged commit bc1e2a4 into minutebase:master Jun 6, 2024
13 checks passed
@RobbieTheWagner
Copy link
Collaborator

Thanks @mkszepp! @Exelord I tried to publish 5.0.0 and it appears I do not have permissions on npm. Can you please grant me permissions on npm? I am rwwagner90 on there

@mkszepp mkszepp deleted the cleanups branch June 6, 2024 12:29
@mkszepp
Copy link
Contributor Author

mkszepp commented Jun 13, 2024

@Exelord friendly ping.
If its not possible to give permission to @RobbieTheWagner it would nice, if you can publish v5 to npm

@RobbieTheWagner
Copy link
Collaborator

@mkszepp I got things released, but I am hitting some issues. It seems like our test generators are not creating tests that work. Like this.owner.lookup('ability:foo') does not find the ability.

@mkszepp
Copy link
Contributor Author

mkszepp commented Jul 2, 2024

@RobbieTheWagner hmm... tested last v5 release and we are running in a issue with pluralization... not sure why addon test app works fine... looks like we need in apps the initializer... which we have removed while v2 addon migration... see #166 (comment)

i will look how we can bring this inside v2 addon. Maybe this is only needed, because the service is plurale 'abilities' instead of 'ability', but this plural service was introduced while v4... see #152

@mkszepp
Copy link
Contributor Author

mkszepp commented Jul 2, 2024

Looking the code-part of initializer it seems to me also a little bit "hacky"... there was added a resolver inside a initializer and the initializer was exported as empty function :(

https://github.com/minutebase/ember-can/blob/v4.2.0/addon/initializers/setup-ember-can.js

import Resolver from 'ember-resolver';

Resolver.reopen({
  init() {
    this._super(...arguments);
    this.set('pluralizedTypes.ability', 'abilities');
  },
});

export function initialize() {}
export default { initialize };

@RobbieTheWagner
Copy link
Collaborator

How does the actual code resolve which ability to use? We should probably just recommend the same thing for tests.

@mkszepp
Copy link
Contributor Author

mkszepp commented Jul 2, 2024

the issue is not only in tests... also when you use (can "create post") it isn't working with v5... i'm trying atm to find a way to bring a resolver inside a v2 addon

@mkszepp
Copy link
Contributor Author

mkszepp commented Jul 2, 2024

in test-app there was never tested a ability :( i have now added one, so maybe i can find a fix

@mkszepp
Copy link
Contributor Author

mkszepp commented Jul 2, 2024

@RobbieTheWagner here a possible fix for our issue.. see #176. i haven't found no other fix... it looks like initializer exports are not working in v2...

I think this is also a clean way instead of current way

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

Successfully merging this pull request may close these issues.

2 participants