Enngage
commented
1 year ago @JiriLojda, thank you very much for such a thorough review :) I'll be going through that as soon as I can.
Closed JiriLojda closed 1 year ago
Enngage
commented
1 year ago @JiriLojda, thank you very much for such a thorough review :) I'll be going through that as soon as I can.
Enngage
commented
1 year ago It took some time, but I've finally been able to go back to this :) Thanks again @JiriLojda for very valuable input. I've implemented most of the recommendations :)
There are some things that this library doesn't intend to do such as using Management API to sync models between projects (which is actually really difficult to do via API). However, It should solve scenarios such as "export data fast in a certain format" and "import content from a certain format".
Review notes:
csvin the name means the file format. I expect this code to be independent on the output format as I can use a different format then csv.Readonly<Record<ElementType, TransformFnc>>, you would not only get a better performance when searching for the matching transformer. Most importantly, TypeScript would make sure, that each element type has exactly one transformer. Then, if we can either relay on Kontent.ai always returning a correctElementTypeand there would be no need to check a transformer exists because that would be ensured at compile-time. Or the check can remain, but it would only be triggered in an unlikely case of Kontent.ai returning an invalidElementType. Either way less opportunity for a mistake in the code here.itemType'component').typesmethod and load all types at once (I know it could be multiple requests but at least a single stream of continuations) instead of iterating it? I am not sure if something similar exists for languages.!!yourVariable..filtercall that will be calling theconfig.canImportfunctions. The number of removed items/assets can then be easily computed by subtracting the filtered array length from the original array length. As a bonus to being more readable, in my opinion, the algorithm will be linear instead of quadratic.console.error..then(r => r.data)instead, there would be no need for wrapping the awaited statement with braces and the formatting would look a bit better imho.ifs and negating them would help you avoid this Egyptian pyramid.const zipSizeInBytes: number = zipData instanceof Blob ? zipData.size : zipData.byteLength;.Promise<number>, but semantically I would say it returns typePromise<void>. I would certainly avoid using typeany. Same to the parameter of the callback you pass into thePromiseconstructor. (i.e.resolve) You don't need to supply it with an explicit type. TypeScript is capable of inferring it from the type of thePromiseconstructor. If you'd really like to provide it with an explicit type a more appropriate type is(res: T) => voidwhereTis the type in the promise, in this casenumber, but I think it is much better to let the TypeScript infer the type here.isFileFromDirectory(dirName: string)function comes to mind..zipfile (the csv or json file would be within the zip), but can import.zipor.csvor.jsonas the root file. Am I correct? I guess there is an option missing to export just a single.jsonor.csvfile.file-processor.service. To me it seems it is a zip-file-handler with some csv and json capabilities inserted in. What would you think about having a file-type handler files (zip, csv, json...) each being able to export/import to/from a relevant file type. The zip handler would need to have a parameter that would create/parse files that are inside, but it would not assume it is a csv or json. The handler would be directly called in the app based on the file extension of the imported file or the option for export.nameortypeas codename?&&condition seem to be the same. I suppose that is a mistake.foreachcycle and scrap the index tracking? Something likeconst headerRow = parser.read();type,name...). Should we validate they are present and inform the user that the file is invalid because of ...?.mapfunction already creates a new array on it's own.unknowninstead ofany. That applies to almost all usages ofany. The best option is to use a specific type you really return so that TypeScript can really do its job. If you don't know the type you are returning,unknownis better thanany, because withanyyou are basically turning TypeScript of for that property/variable/constant. Withunknown, on the other hand, you are telling TypeScript that you don't know what the type is and it will make sure, that you will assert the type at runtime before you use the reference anywhere. Technically,unknownis not assignable to anything and if you want to use it in any way you need to assert it to a certain type ideally with type guards.('field1' | 'field2')[]or even better['field1', 'field2'], you wouldn't need to convert theparsedAssettoanyand therefore use the type safety. Of course that would mean to validate the field names properly which is a good idea anyway.filter(m => !!m.codename?.length).file-helper.tsdoesn't seem to be used.configparameter doesn't seem to be used.iflike so.if (condition) {throw {...}; } throw error;csv-.... Also, I think I mentioned it earlier, I expected to be able to export into a pure json, not a zip with json files.