[ENHANCEMENT] Change blueprint command options to type String to avoid nopt transformations #3820
Conversation
it seems like people being required to provide a relative or absolutely path, should be sufficient we dont need to resolve non-path like things. I believe https://github.com/ember-cli/ember-cli/blob/master/tests/acceptance/new-test.js#L130 should fail in its current form, but work with |
|
Sure, that sounds very reasonable. How would we technical detect it though? Check for the presence of '/' in the blueprint name and assume it's a path? |
ya, relative or absolute. |
rodyhaddad
force-pushed
the
blueprint-option-as-string
branch
2 times, most recently
from
87d5d79
to
ac13a3a
Compare
|
@stefanpenner I updated the PR and added an extra test for absolute paths. Let me know if there's anything I should change before merging. |
rodyhaddad
force-pushed
the
blueprint-option-as-string
branch
from
ac13a3a
to
d1cd079
Compare
|
looking now |
| @@ -72,6 +72,10 @@ module.exports = Command.extend({ | |||
| return Promise.reject(new SilentError('We currently do not support a name of `' + packageName + '`.')); | |||
| } | |||
|
|
|||
| if (blueprintOpts.blueprint[0] === '.') { | |||
| blueprintOpts.blueprint = path.resolve(process.cwd(), blueprintOpts.blueprint); | |||
There was a problem hiding this comment.
at first, I thought this was a mistake. But I believe it to be correct, as you want:
cd app/ && ember --blueprint=../blueprints/footo be relative the cwd, not the project root.
👍
|
1 last comment, and then i feel good to merge this. |
…rmations This is alright, as the check for `gitUrl` happens in the install-blueprint task anyway.
rodyhaddad
force-pushed
the
blueprint-option-as-string
branch
from
d1cd079
to
413e311
Compare
|
@stefanpenner Tada! |
[ENHANCEMENT] Change blueprint command options to type String to avoid nopt transformations
Before this PR, the
--blueprintoption in thenewandinitcommands got validated as agitUrlor apath.When used as a
path, it would get resolved to an absolute path, making it point to a specific blueprint on the filesystem depending on the current working directory.This PR changes it to a String, so it won't be modified before getting lookup'ed up by
Blueprint.lookupThis doesn't break it being used as a
gitUrl, because that check happens again in the InstallBlueprint task.It does break it from being used as a path to a blueprint on the filesystem. Instead now it's used as a blueprint in one of the regular lookup paths.
I don't believe this will affect many people, if any. I imagine people always rely on the default
appblueprint when usingneworinit.I considered keeping it backward compatible, but the new behaviour might be what we actually want moving forward.
The only test this breaks is this one, which I'm for removing/modifying, but wanted someone else's opinion.
\cc @stefanpenner