Separate native and JS prepare code #3116
Conversation
nadyaA
requested review from
Mitko-Kerezov,
rosen-vladimirov and
TsvetanMilanov
lib/definitions/platform.d.ts
Outdated
|
|
||
| interface IPreparePlatformNativeService extends IPreparePlatformService { | ||
|
|
||
| } |
lib/definitions/platform.d.ts
Outdated
| preparePlatform(platform: string, platformData: IPlatformData, appFilesUpdaterOptions: IAppFilesUpdaterOptions, projectData: IProjectData, platformSpecificData: IPlatformSpecificData, changesInfo?: IProjectChangesInfo, filesToSync?: Array<String>, projectFilesConfig?: IProjectFilesConfig): Promise<void>; | ||
| } | ||
|
|
||
| interface IPreparePlatformJSService extends IPreparePlatformService { |
There was a problem hiding this comment.
what's the purpose of the empty interfaces?
| import { AppFilesUpdater } from "./app-files-updater"; | ||
| import { EventEmitter } from "events"; | ||
|
|
||
| export class PreparePlatformService extends EventEmitter { |
There was a problem hiding this comment.
Why do you extend EventEmitter, there's no emit anywhere in the services?
| super($fs, $xmlValidator); | ||
| } | ||
|
|
||
| public async addPlatform(platformData: IPlatformData, frameworkDir: string, installedVersion: string, projectData: IProjectData, config: IPlatformOptions, platformTemplate: string, ): Promise<void> { |
There was a problem hiding this comment.
the comma after platformTemplate: string is incorrect
There was a problem hiding this comment.
I know the code is extracted from already existing one, but this method has too many parameters. Can you pass an object instead
Mitko-Kerezov
force-pushed
the
natanasova/prepare-service
branch
5 times, most recently
from
bd17fd6 to
9495236
Compare
|
Ping @nadyaA @rosen-vladimirov @TsvetanMilanov with some additional implementation |
lib/definitions/platform.d.ts
Outdated
| * @param {IDeployPlatformOptions} deployOptions Various options that can manage the deploy operation. | ||
| * @param {IProjectData} projectData DTO with information about the project. | ||
| * @param {IAddPlatformCoreOptions} config Options required for project preparation/creation. | ||
| * @param {IDeployPlatformInfo} deployInfo Options required for project preparation. |
There was a problem hiding this comment.
I think there could be better definition for this interface as it is called deploy smth and nothing about deployment is said in the definition 🤔
| @@ -293,15 +255,35 @@ export class PlatformService extends EventEmitter implements IPlatformService { | |||
|
|
|||
| /* Hooks are expected to use "filesToSync" parameter, as to give plugin authors additional information about the sync process.*/ | |||
| @helpers.hook('prepare') | |||
| private async preparePlatformCore(platform: string, appFilesUpdaterOptions: IAppFilesUpdaterOptions, projectData: IProjectData, platformSpecificData: IPlatformSpecificData, changesInfo?: IProjectChangesInfo, filesToSync?: Array<String>, nativePrepare?: INativePrepare): Promise<void> { | |||
| private async preparePlatformCore(platform: string, appFilesUpdaterOptions: IAppFilesUpdaterOptions, projectData: IProjectData, platformSpecificData: IPlatformSpecificData, env: Object, changesInfo?: IProjectChangesInfo, filesToSync?: string[], nativePrepare?: INativePrepare): Promise<void> { | |||
There was a problem hiding this comment.
Adding new mandatory parameter changes method signature - is this okay with people using the hook?
There was a problem hiding this comment.
Yeah, this should be fine, people using the hook, use the name, for example hookArgs.filesToSync: https://github.com/NativeScript/nativescript-dev-sass/blob/master/lib/after-prepare.js#L17
| @@ -15,6 +15,7 @@ export class Options extends commonOptionsLibPath.OptionsBase { | |||
| forDevice: { type: OptionType.Boolean }, | |||
| provision: { type: OptionType.Object }, | |||
| client: { type: OptionType.Boolean, default: true }, | |||
| env: { type: OptionType.Object }, | |||
There was a problem hiding this comment.
what about uglify?
There was a problem hiding this comment.
uglify will be passed as a regular options - --env.uglify, we've agreed with @sis0k0 on that behavior
There was a problem hiding this comment.
The same applies for --env.snapshot
|
run ci |
Create interfaces to separate the prepare functionality in js and native parts. Ensure better isolation and encapsulation of project preparation process.
Mitko-Kerezov
force-pushed
the
natanasova/prepare-service
branch
from
9495236 to
681512a
Compare
* Extract all code that webpack needs to replace into a single method * Introduce a hook (`prepareJSApp`) for that method so it can be replaced * Add additional check for when not to delete `App_Resources` directory when preparing JS so that whenever preparing natively we don't copy all the files twice
Mitko-Kerezov
force-pushed
the
natanasova/prepare-service
branch
from
681512a to
1dd1398
Compare
Create interfaces to separate the prepare functionality in js and native parts.
Ensure better isolation and encapsulation of project preparation process.
Merge after telerik/mobile-cli-lib#1018
#2968