Skip to content

Conversation

@jamescdavis
Copy link
Member

resolve #146

Copy link
Member

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

This looks good to me overall. I’d love input from @dfreeman and/or @dwickern as well, as both have way more experience dealing with in-repo-addons than I do. I do also wonder if the "ember-cli": "*" constraint in the package.json will ever cause issues in terms of what gets installed and our test matrix. I don’t think so? But I’m not positive, and am thinking we should probably specify an actual version.

@jamescdavis jamescdavis force-pushed the update-tsconfig-when-in-repo-addons-are-generated branch from 5d5f014 to da5fb96 Compare February 25, 2018 06:32
@jamescdavis
Copy link
Member Author

Yeah, I'm wasn't happy with "ember-cli": "*" in dependencies, either. I moved to peerDependencies as this also pleases eslint, yet shouldn't ever affect what version gets installed.

I would, though, rather have even this specify version, but didn't initially because I wasn't sure of the minimum version ember-cli-typescript supports? "ember-cli": ">=2.12"? But maybe should be "ember-cli": "^2.12.0 || ~3.0.0" and then bump versions as each new ember-cli minor release tests clean? Or could go with "ember-cli": "^2.12.0 || ^3.0.0" and just assume it'll work until 4.0 (because, ya know, we all follow semver perfectly, right? ;).

@jamescdavis jamescdavis force-pushed the update-tsconfig-when-in-repo-addons-are-generated branch from da5fb96 to 5202b38 Compare February 26, 2018 15:38
Copy link
Member

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

This looks great! The one question I have is whether it's possible/reasonable to share any of the logic around setting up tsconfig.json for in-repo addons between this blueprint and the ember-cli-typescript one that's executed on installation. It may be that things are stable enough now that it's not a big deal, but currently this means we'll have to be diligent about updating both places if the details of that ever change.

As far as the ember-cli peerDep goes, I've just told ESLint I know better for the specific case of require()ing modules from ember-cli up to this point. Adding the peer dependency won't hurt anything, but realistically I think it's also not likely to catch erroneous installation states either, regardless of whether we go with * or a particular version range.

@jamescdavis jamescdavis force-pushed the update-tsconfig-when-in-repo-addons-are-generated branch 3 times, most recently from 59a95ab to 086ff79 Compare March 1, 2018 16:15
@jamescdavis
Copy link
Member Author

@dfreeman I'm always a fan of DRYing things ups, so I moved the code for updating tsconfig.json paths to a utility and used it both in the in-repo-addon and ember-cli-typescript blueprints. I also added an option to the updatePathsForAddon utility that will do the right thing for linked addons so that we can use this for a command that sets up linked addons.

I agree that an ember-cli peer dependency doesn't really help much, so I've switched to eslint-disable-line node/no-unpublished-require.

@jamescdavis jamescdavis force-pushed the update-tsconfig-when-in-repo-addons-are-generated branch from 086ff79 to f818740 Compare March 2, 2018 17:44
@dfreeman
Copy link
Member

dfreeman commented Mar 5, 2018

@chriskrycho I know you're swamped this week, but if you happen to have a few minutes of downtime at some point, it looks like Appveyor just went out to lunch on the builds for this and #159 and never reported in, and I think you're the only person who can re-kick them.

@chriskrycho
Copy link
Member

chriskrycho commented Mar 5, 2018 via email

@chriskrycho
Copy link
Member

Kicked. We'll see if it reports properly this time. If not, I'm just going to merge it, since it passes.

@dfreeman dfreeman merged commit d9e11c6 into typed-ember:master Mar 5, 2018
@jamescdavis jamescdavis deleted the update-tsconfig-when-in-repo-addons-are-generated branch March 7, 2018 14:57
dfreeman added a commit that referenced this pull request Aug 21, 2018
…-addons-are-generated

Update tsconfig when in-repo addons are generated
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.

Automatically update tsconfig.json when in-repo addons are generated

3 participants