Separate addon and test app with into Monorepo #907
Conversation
|
looks great! I'll let @elwayman02 weigh in if any concerns, otherwise merge by EOD tomorrow. |
addon/.eslintrc.js
Outdated
| @@ -38,7 +38,6 @@ module.exports = { | |||
| './testem.js', | |||
| './blueprints/*/index.js', | |||
| './config/**/*.js', | |||
| './tests/dummy/config/**/*.js', | |||
There was a problem hiding this comment.
Does this file still need the "test fles" section on lines 51-58?
There was a problem hiding this comment.
@elwayman02 we indeed do not need. Also there bunch of other things that could be cleaned up, but my thought process is to get this land with minimal actual changes in file and focus on moving files into locations where they should be.
as next step, we'll sync test-app with latest ember-cli app blueprint and sync addon folder setup with https://github.com/embroider-build/addon-blueprint after we conver the actual addon to v2 format.
Sorry didn't communicate this plan before, hope this makes sense as with such approach we should have more or less easily reviewable PRs.
There was a problem hiding this comment.
@elwayman02 I removed the test file overrides
package.json
Outdated
| "ember-sinon" | ||
| ] | ||
| "concurrently": "^8.2.1", | ||
| "@release-it-plugins/lerna-changelog": "^6.0.0" |
There was a problem hiding this comment.
What's the reasoning to split up this plugin dependency from the release-it dependency?
There was a problem hiding this comment.
ah good catch! We indeed need to have release-it in root level package.json in devDependencies.
There was a problem hiding this comment.
@SergeAstapov, @elwayman02 my mistake, it should be fixed now
addon/package.json
Outdated
| }, | ||
| "ember-addon": { | ||
| "after": [ | ||
| "ember-sinon" |
There was a problem hiding this comment.
Can you go ahead and drop this? We should have dropped it before, but we no longer rely on ember-sinon so this block is useless.
There was a problem hiding this comment.
It's also ok for this to be a follow-up, just wanted to call it out while I noticed it.
| @@ -6,8 +6,8 @@ | |||
| "version": "4.11.0", | |||
| "blueprints": [ | |||
| { | |||
| "name": "addon", | |||
| "outputRepo": "https://github.com/ember-cli/ember-addon-output", | |||
| "name": "app", | |||
There was a problem hiding this comment.
Hmm, I'm not sure about this change. How exactly does ember-cli-update work with v2 addons? We still want to be able to apply the v2 addon blueprint with this config and have it affect both the addon and test-app directories.
There was a problem hiding this comment.
@elwayman02 this is actually correct change.
this file will be used only for test app so we could do npx ember-cli-update in the test-app folder.
for the addon and everything, there will be separate config/ember-cli-update.json file which we would be able to use to run npx ember-cli-update in the root folder (you could see the template for that file in https://github.com/embroider-build/addon-blueprint/blob/main/files/config/ember-cli-update.json)
There was a problem hiding this comment.
Ok, so this file is fine but we need to add a new config/ember-cli-update.json as well then, right?
There was a problem hiding this comment.
Yeap, but I would do in another PR as it would not make any sense just yet
|
Let's fix the release-it dependency and the other 3 comments can be follow-up PR(s). This looks great, thanks for putting in the hard work! |
No description provided.