Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfix(@schematics/angular): add appDir option #17816
Conversation
| const sourceRoot = clientProject.sourceRoot || 'src'; | ||
| const serverPath = options.appDir ? | ||
| `${sourceRoot}/${options.appDir}` : | ||
| sourceRoot; |
SchnWalter
May 28, 2020
•
Contributor
L.E. Sorry, I've mistaken this for a schematic that adds an app-shell model from the Android world for the application, not for the server side. You can ignore most of what I've wrote here.
This change needs a more thorough analysis. I think that appDir option should be removed - it didn't work in the first place, so just deprecate it now and remove it later, and there is no harm done.
In other schematics, buildDefaultPath() is used to hardcode an app or lib sub-directory (depending on the project type) and it will come into conflict with this; you want other schematics to also work for projects that have used the app-shell schematics.
Alternatively, this could be implemented for all schematics, and have it fallback to the current behavior of buildDefaultPath(). I've actually submitted a similar feature suggestion, but for removing the sub-directory for all schematics, see #17598.
But I'm not sure if either of these should be implemented, we need to make sure that existing tutorials on how to generate components will continue to work in the future (including for projects that have used app-shell).
santoshyadavdev
force-pushed the
santoshyadavdev:fix--add-appDir-option-in-appShell
branch
4 times, most recently
from
5fe0bf3
to
e437829
|
@alan-agius4 it's open for review. |
|
@santoshyadav198613, I’ll review it tomorrow. Thanks. |
|
Hi @santoshyadav198613, Firstly thanks for the PR This changes however doesn't seem to be addressed the problem in the right place. The problem with appDir is actually located in the universal schematic. In fact, it's right that the app-shell schematic is failing, because At the very least there are two places that you need to change.
|
| @@ -10,6 +10,21 @@ import { Schema as ApplicationOptions } from '../application/schema'; | |||
| import { Schema as WorkspaceOptions } from '../workspace/schema'; | |||
| import { Schema as AppShellOptions } from './schema'; | |||
|
|
|||
| function createServerFile(appTree: UnitTestTree): void { | |||
santoshyadavdev
Jun 2, 2020
Author
Contributor
Ohh got it, So my impression was incorrect, I will make the changes.
| @@ -33,8 +33,7 @@ | |||
| "appDir": { | |||
| "type": "string", | |||
| "format": "path", | |||
| "description": "The name of the application directory.", | |||
| "default": "app" | |||
|
Hi @alan-agius4 , |
|
@santoshyadav198613, yes I believe so. The We might need to use the |
santoshyadavdev
force-pushed the
santoshyadavdev:fix--add-appDir-option-in-appShell
branch
from
e437829
to
2924bc7
santoshyadavdev
force-pushed the
santoshyadavdev:fix--add-appDir-option-in-appShell
branch
6 times, most recently
from
344e6fd
to
4ec8641
santoshyadavdev
force-pushed the
santoshyadavdev:fix--add-appDir-option-in-appShell
branch
3 times, most recently
from
4fb9216
to
a5fa1ec
santoshyadavdev
force-pushed the
santoshyadavdev:fix--add-appDir-option-in-appShell
branch
10 times, most recently
from
664dbf4
to
1b41589
santoshyadavdev
force-pushed the
santoshyadavdev:fix--add-appDir-option-in-appShell
branch
from
1b41589
to
ba44abc
|
Hi @alan-agius4 , |
| it('should create the shell component with appDir option provided', async () => { | ||
| const defaultOptions: AppShellOptions = { | ||
| clientProject: 'bar', | ||
| appDir: '/projects/bar/src/app/server', |
alan-agius4
Jun 10, 2020
Collaborator
appDir should be a directory example myApp and not a path in the workspace.
Also, expect(tree.exists('/projects/bar/src/app/app-shell/app-shell.component.ts')).toBe(true); doesn't seem good, because the app-shell should be created in the provided appDir.
|
@santoshyadav198613, will try to look at this in the coming days. |
|
@santoshyadav198613, just to let you know that I didn’t forget about this! Apologies for taking so long to look at this. |
fixes #17813