-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Make sure a custom blueprint will be used if it's not the default blueprint #1084
Make sure a custom blueprint will be used if it's not the default blueprint #1084
Conversation
70d622f
to
a6763b8
Compare
59223b8
to
4d3d7b5
Compare
@spham92 - Looks like we have a conflict over in:
|
modify "can install an addon with a default blueprint and a state file" to follow new expectations
cea2a59
to
6982022
Compare
FWIW, the last failing job is due to the issue I created #1085 to track (fork PRs can never pass CI at the moment). |
@kellyselden can you take a look at the PR when you get a chance? |
Co-authored-by: Jordan Hawker <[email protected]>
ping @kellyselden |
ping @kellyselden |
|
||
let generate; | ||
|
||
if (!blueprintPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this logic block in the new version. Is that going to be a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if I missed a use case.
I moved the responsibility of choosing which blueprint to use up to the parent function that calls installAndGenerateBlueprint
.
install
command will default to the addon name if a blueprint
param is not passed by the user.
line 45-53 of src/install.js
let { ps } = await installAndGenerateBlueprint({
cwd,
addonNameOverride: addon,
packageName,
blueprintPath: path
blueprintName: _blueprintName || addon,
blueprintPath: path,
packageManager: isYarnProject ? 'yarn' : 'npm'
});
installAddonBlueprint
also will pass a blueprintName
param
line 275-284 of src/get-start-and-end-command.js
let { ps } = await installAndGenerateBlueprint({
cwd: projectRoot,
packageName: blueprint.packageName,
version: blueprint.version,
blueprintPath: blueprint.path,
stdin: 'pipe'
blueprintName: blueprint.name,
blueprintOptions: blueprint.options || [],
stdin: 'pipe',
packageManager
});
module.exports = installAndGenerateBlueprint; | ||
module.exports.ember = ember; | ||
module.exports.resolvePackageName = resolvePackageName; | ||
module.exports.spawn = spawn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you don't mind double checking all of these that can be removed now that the test isn't stubbing anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need these for the actual unit test of this module
Looking good. I have a few more suggestions. I checked out the code and stepped through the tests, so I have a better understanding of the functionality now :) |
@kellyselden current state of code addresses the PR comments |
"version": "0.0.1", | ||
"blueprints": [ | ||
{ | ||
"name": "custom-blueprint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the initial install, will someone be able to update this blueprint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or is it correct that after the initial resolution of the blueprint name from install
, we no longer have to worry about it being a name mismatch (and we never have to look it up again)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter is correct. Once the blueprint name is set in ember-cli-update.json
it will be the one generated when user wants to update the package version the blueprint belongs to.
Friendly reminder @kellyselden |
@@ -45,4 +45,9 @@ describe(getBlueprintNameOverride, function() { | |||
let defaultBlueprintOverride = await getBlueprintNameOverride(localPackageFixture); | |||
expect(defaultBlueprintOverride).to.be.equal('custom-blueprint'); | |||
}); | |||
|
|||
it('NPM package with nondefault returns expected value', async function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick but this test should be moved to a new file test/integration/get-default-blueprint-name-override-test.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as well as the above one which is testing the disk blueprints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
Bug
I wanted to update the state of my project with changes to a blueprint from an Ember addon package that is not the default:
When I run
ember-cli-update
and choose@stpham/bootstrapping-project
, the@stpham/generic-addon
version is bumped but the actual blueprint changes aren't there. i.e. file additions.Root Cause
It seems like the code assumes the default blueprint would be the only blueprint that would be used
The
generate = ember(['g', packageName], { cwd, stdin });
line will only be hit if theember-cli
version is deemed to be a buggy one else it only usesember install <package>
.Changes
End User Experience
After these changes are pushed, the end-user will be required to provide a blueprint name with the
install
command if they want to use another blueprint that doesn't match the addon name.ember-cli-update install <package_name>
will assume there exists a blueprint with the same name as<package_name>
.ember-cli-update install <package_name> -b <some_other_blueprint>
to install package andember g <some_other_blueprint>
.We should release version 1.0.0 to indicate the above requirement exists for custom blueprints. Judging that this problem has existed for a while, I don't think many folks use ember-cli-update for custom blueprints
Code Changes
I opted to move away from using
ember install <package_name>
to install packages since it will automatically try to generate the default blueprint. Since there will always be a blueprint name in theember-cli-update.json
tied to a package (I assume), I figured the code can be more explicit in what it is doing. I refactored and renamedember-install-addon.js
toinstall-and-generate-blueprint.js
and made it always install a package and generate the specified blueprint.