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 addon to V2 #218

Merged
merged 16 commits into from
Feb 5, 2024
Merged

Convert addon to V2 #218

merged 16 commits into from
Feb 5, 2024

Conversation

mkszepp
Copy link
Contributor

@mkszepp mkszepp commented Jan 12, 2024

Fix #217

  • Update addon to V2
  • Allow ember-source 3.28, 4.x and >5.0

@mkszepp
Copy link
Contributor Author

mkszepp commented Jan 12, 2024

@jelhan i think this is ready... please approve workflow, so we can check of all tests do pass

@jelhan
Copy link
Owner

jelhan commented Jan 12, 2024

Thanks a lot for working on picking this up that quickly!

How did you converted it? Is it based on Embroider addon blueprints?

I noticed some misalignments with Ember CLI conventions. E.g. Prettier configuration is different. If this mismatch is caused by the blueprints, I would report upstream rather than discussing in this PR. That's why I'm asking.

@mkszepp
Copy link
Contributor Author

mkszepp commented Jan 13, 2024

@jelhan for v2 addon migration i have always used https://github.com/NullVoxPopuli/ember-addon-migrator this is using embroider blueprint internal

The migrator has some problems, like readme, changelog or renovate will not be readded correctly. Also docs will be lost completly... This is a manual step for the moment.
Also while migration i have got errors by porting other addons (this one was going very simple). This is atm a beta, but it is helpful.

The migrator make +- new apps and is readding all files from before.

I will check the prettier file... because i think he has hold the standard files, not the file from repo

Edit:
i have now readded some settings... i think we have now everything or?

@jelhan
Copy link
Owner

jelhan commented Jan 13, 2024

Thanks a lot for providing the details. If changes are due to different opinions of Ember CLI and Embroider blueprints that's fine for me and nothing which needs to be changed here. I will report those upstream and of there is a decision to align, it should propagate to this repo in next Ember CLI Upgrade.

Prettier is one example. Ember CLI configures Prettier to use double quotes as standard. And only overwrites for JavaScript and TypeScript files to use single quotes instead. (Source) Embroider addon blueprints configure Prettier to use single quotes for all files. (Source) For this PR both is fine for me. But I think long-term it should be aligned between the blueprints.

I hope I have some time doing a full review this weekend. Otherwise I will get back to it early next week.

@mkszepp
Copy link
Contributor Author

mkszepp commented Jan 24, 2024

@jelhan did you have time for review?

Copy link
Owner

@jelhan jelhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reminder. I'm sorry for not getting back to this earlier. I didn't had much time for open source recently. And reviewing the v2 conversion was a rather complex task due to amount of changes.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.prettierrc.cjs Outdated Show resolved Hide resolved
ember-style-modifier/.prettierrc.cjs Show resolved Hide resolved
ember-style-modifier/src/template-registry.ts Outdated Show resolved Hide resolved
test-app/package.json Outdated Show resolved Hide resolved
test-app/package.json Outdated Show resolved Hide resolved
test-app/package.json Outdated Show resolved Hide resolved
test-app/package.json Outdated Show resolved Hide resolved
test-app/package.json Outdated Show resolved Hide resolved
@mkszepp mkszepp requested a review from jelhan February 1, 2024 07:13
@mkszepp
Copy link
Contributor Author

mkszepp commented Feb 1, 2024

@jelhan please don't merge renotate bot changes atm... because its not possible to merge this changes. i have now updated manually the merged dependency updates

Copy link
Owner

@jelhan jelhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for keeping working on this!

Seems all good beside one bug in GitHub Actions and one code formatting issue for YAML files.

CI pipeline is failing because one recently added file wasn't formatted with Prettier.

I haven't tested locally the created package yet. Will do that as a last step before merging.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@mkszepp mkszepp requested a review from jelhan February 4, 2024 17:23
@mkszepp
Copy link
Contributor Author

mkszepp commented Feb 4, 2024

@jelhan hope the test do pass now, otherwise let me know

Copy link
Owner

@jelhan jelhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for pushing this over the finish line!

@jelhan jelhan merged commit b40d6df into jelhan:master Feb 5, 2024
15 checks passed
@jelhan jelhan added the enhancement New feature or request label Feb 5, 2024
@jelhan
Copy link
Owner

jelhan commented Feb 5, 2024

Released as v4.2.0.

@mkszepp mkszepp deleted the convert-to-v2-addon branch February 6, 2024 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert addon to v2 & allow older ember-source versions than 4.12
2 participants