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 #87

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

jrjohnson
Copy link
Contributor

Ran pnpx ember-cli@latest addon @ember/render-modifiers -b @embroider/addon-blueprint --pnpm --skip-install --skip-git --typescript --addon-location=ember-render-modifiers and created everything from scratch then copied in modifiers, tests, and the PNPM version setting from ci.yml

Superseeds #66

Todo:

  • I'm not a typescript user, I used the typescript blueprint, but I don't know how to move the current types into this code. I'd assume declare them officially in the modifiers, but I don't know how to do that.
  • I'm not a release-it user and it doesn't seem to be part of the add-on blueprint anymore, this will need to be configured to work with the ember process.

@jrjohnson jrjohnson mentioned this pull request Mar 17, 2025
Ran `pnpx ember-cli@latest addon @ember/render-modifiers -b
@embroider/addon-blueprint --pnpm --skip-install --skip-git --typescript
--addon-location=ember-render-modifiers` and created everything from
scratch then copied in modifiers, tests, and the PNPM version setting
from ci.yml
@Windvis
Copy link

Windvis commented Mar 19, 2025

Awesome, thank you for working on this!

I'm not a typescript user, I used the typescript blueprint, but I don't know how to move the current types into this code. I'd assume declare them officially in the modifiers, but I don't know how to do that.

v2 addons are configured to use the /declarations folder instead of the /types folder, so you can probably move them there, which most likely needs some gitignore changes. There is also a declarations plugin in the rollup config file which might need to be disabled, since they aren't generated from the source.

I'm not a release-it user and it doesn't seem to be part of the add-on blueprint anymore, this will need to be configured to work with the ember process.

Most official Ember projects now use release-plan, which can only be setup by maintainers AFAIK, so I don't think you need to do anything release related in this conversion PR.

Comment on lines +65 to +66
{ src: '../../README.md', dest: '.' },
{ src: '../../LICENSE.md', dest: '.' },
Copy link

Choose a reason for hiding this comment

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

I think these files are stored in the parent folder?

Suggested change
{ src: '../../README.md', dest: '.' },
{ src: '../../LICENSE.md', dest: '.' },
{ src: '../README.md', dest: '.' },
{ src: '../LICENSE.md', dest: '.' },

"webpack": "^5.95.0"
},
"peerDependencies": {
"@glint/template": "^1.0.2",
Copy link

Choose a reason for hiding this comment

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

This peerDep is probably still needed (together with the peerDependenciesMeta config).

},
"dependencies": {
"@embroider/addon-shim": "^1.8.9",
"@embroider/util": "^1.0.0",
Copy link

Choose a reason for hiding this comment

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

Why is this dep needed? I don't think it's used, but hard to say for sure from the web UI 😅.

@Windvis

This comment was marked as outdated.

"@tsconfig/ember": "^3.0.8",
"babel-plugin-ember-template-compilation": "^2.2.5",
"concurrently": "^9.0.1",
"ember-source": "^5.4.0",
Copy link

Choose a reason for hiding this comment

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

This is the reason why v5.12 is installed and the Embroider scenario fails. I think you can fix it by making it match the test-app version.

@jrjohnson
Copy link
Contributor Author

@Windvis thanks for the review. I think, but haven't gone back to confirm yet, that all of these issues are right off the embroider addon blueprint. My feeling is we should always stick as closely to the blueprint as possible to make updating easy.

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.

2 participants