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

Convert to V2 addon #166

Merged
merged 22 commits into from
May 23, 2024
Merged

Conversation

mkszepp
Copy link
Contributor

@mkszepp mkszepp commented May 13, 2024

Now, as we have a modern app, its time to move to an addon V2 (RFC)

Changes:

  • convert addon to v2 (using addon-blueprint)
  • convert helpers to TS
  • add template registry
  • replace model type to simple object
  • Modify instanceof check (caused by ember-auto-import bug see below)
  • removing initializer (see comment below)

When this changes are landed we should do:

  • drop ember < 3.28
  • removing codes deprecation like 'can' service & computed.js (after that, the addon is typed)

close #159

@mkszepp
Copy link
Contributor Author

mkszepp commented May 13, 2024

We are running in this issue: embroider-build/ember-auto-import#588 (instanceof is not working correctly in tests)
To make a simple workaround while this will be fixed i have checked if there are existing necessary functions... see commit d0bd2fa

An alternative is to hold class in an global context since this bug is fixed... let me know if this workaround is okay for now, or if e need to remove it

@mkszepp
Copy link
Contributor Author

mkszepp commented May 13, 2024

We have a initializer inside our addon... this doesn't exists in V2 addons... i'm also not sure, if we really need it, because plural of ability is abilities... idk why this was added... maybe there was a bug in any old version... (commit of remove 0b8d5e6)

Atm i have removed it... let me know if we need this...
When we need, the consumer app must make something in his app.js (importing this initializer). Magically like in v1 addon isn't not anymore working

@mkszepp mkszepp marked this pull request as ready for review May 13, 2024 10:49
@mkszepp
Copy link
Contributor Author

mkszepp commented May 13, 2024

@esbanarango @RobbieTheWagner CI is now green 🚀... please read my previous comments and let me know whats the plan

@RobbieTheWagner RobbieTheWagner added the breaking Breaking Change label May 22, 2024
@RobbieTheWagner
Copy link
Collaborator

@mkszepp thanks for your work on this! Can you elaborate on if there are actual breaking changes in this? If we think the initializer was not needed, then is this actually breaking? Are we dropping node or Ember versions or changing anything major here? I want to make sure we label the PR correctly.

@mkszepp
Copy link
Contributor Author

mkszepp commented May 22, 2024

@RobbieTheWagner by merging this changes we are breaking, because we have dropped the app initializer. That the only thing, why we are braking in this PR. I'm not sure why this was added (it comes from release version v1 of this addon), maybe there was anytime a problem with pluralizing...

Nodejs version is not anymore necessary to document, because v2 addons are statically... for this reason its also not braking.

Ember Version support:
By closing this PR, we are supporting 3.20+ (like before, no braking change). Tests are green, so i think everything its ok...
I would only like to drop older version < 3.28 in an other PR, that this is clearly documented in release notes. In additional we can remove some test szenarios and also in future we don't need to support a so old version (already v3.28 its old, but i know that peoples are still on 3.28 and the major part of modern addons are still supporting this version)

@charlesfries
Copy link
Contributor

We are also losing the ability blueprints with this PR. I recall reading that blueprints aren't even compatible with V2 addons, but maybe something to mention in the release

@mkszepp
Copy link
Contributor Author

mkszepp commented May 22, 2024

Ups... blueprint we ca readd... the convert script has maybe not implemented to copy into right place..
I will readd the blueprints... thanks for info
Blueprints are working also in v2... already added in basic dropdown/power select...

@mkszepp
Copy link
Contributor Author

mkszepp commented May 23, 2024

blueprints were readded... they are working like before... no braking changes

@mkszepp
Copy link
Contributor Author

mkszepp commented May 23, 2024

blueprints are now in TS... when consumer app has "isTypeScriptProject": true in .ember-cli they will be created as TS, otherwise they will transformed into JS.. this is maybe braking, because not sure how old ember-clis are working with shouldTransformTypeScript: true,

@mkszepp
Copy link
Contributor Author

mkszepp commented May 23, 2024

one thing what we should do (let me know if we want do it here or later in an other PR)...

ability-test blueprint is checking atm if ember-cli-mocha or ember-cli-qunit are installed... Both packages are deprecated since 2019 and replaced with ember-mocha / ember-qunit...
Looking to RFC ember-mocha is also already depreacted starting with ember-source v5 see https://rfcs.emberjs.com/id/0858-deprecate-ember-mocha/

So i think, we should make blueprint changes braking and allow only ember-qunit... what do you think?

_getTestStyle: function () {
if ('ember-cli-mocha' in this.project.addonPackages) {
return 'mocha';
} else if ('ember-cli-qunit' in this.project.addonPackages) {
return 'qunit';
}
},
locals: function (options) {
var name = options.entity.name;
var testStyle = this._getTestStyle();
if (!testStyle) {
this.ui.writeLine("Couldn't determine test style - using QUnit");
testStyle = 'qunit';
}

@RobbieTheWagner
Copy link
Collaborator

@mkszepp I agree we should only support ember-qunit. So you think there are no breaking changes for this PR? Just trying to decide on a label.

@mkszepp
Copy link
Contributor Author

mkszepp commented May 23, 2024

@RobbieTheWagner no its braking, because we have removed initializer that is possible braking for old ember-inflector.

@RobbieTheWagner
Copy link
Collaborator

@RobbieTheWagner no its braking, because we have removed initializer that is possible braking for old ember-inflector.

But I thought you said the initializer was doing nothing? If it was not needed before, I wouldn't consider that breaking.

@mkszepp
Copy link
Contributor Author

mkszepp commented May 23, 2024

But I thought you said the initializer was doing nothing? If it was not needed before, I wouldn't consider that breaking.

Yes it looks so...
So this PR is not braking

@RobbieTheWagner RobbieTheWagner added enhancement and removed breaking Breaking Change labels May 23, 2024
@RobbieTheWagner RobbieTheWagner merged commit 0cf6ef4 into minutebase:master May 23, 2024
15 checks passed
@RobbieTheWagner
Copy link
Collaborator

Okay, I marked as enhancement and merged. Thanks for the PR!

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

Successfully merging this pull request may close these issues.

Interested moving to a V2 addon ?
4 participants