[Bug] The --exclude-file value is interpreted as the file content instead of the file path

emarbo commented 3 months ago

Hi @daniel-sc, thanks for this useful library!

Summary

It seems that the --exclude-file file path is passed directly to the new XmlDocument, but this class expects the file content instead of the file path.

Execution and error

Error command:

npx xliff-simple-merge \
    -i projects/user-console/src/locale/messages.xlf \
    -e projects/shared/src/locale/messages.xlf \
    -d output.xlf

Error output:

/home/earenas/projects/my_project/node_modules/xmldoc/lib/xmldoc.js:92
    throw err;
    ^

Error: Non-whitespace before first tag.
Line: 0
Column: 1
Char: p
    at error (/home/earenas/projects/my_project/node_modules/sax/lib/sax.js:652:10)
    at strictFail (/home/earenas/projects/my_project/node_modules/sax/lib/sax.js:678:7)
    at beginWhiteSpace (/home/earenas/projects/my_project/node_modules/sax/lib/sax.js:952:7)
    at SAXParser.write (/home/earenas/projects/my_project/node_modules/sax/lib/sax.js:1007:11)
    at new XmlDocument (/home/earenas/projects/my_project/node_modules/xmldoc/lib/xmldoc.js:281:17)
    at /home/earenas/projects/my_project/node_modules/xliff-simple-merge/dist/src/merge.js:184:160
    at Array.map (<anonymous>)
    at mergeWithMapping (/home/earenas/projects/my_project/node_modules/xliff-simple-merge/dist/src/merge.js:184:141)
    at merge (/home/earenas/projects/my_project/node_modules/xliff-simple-merge/dist/src/merge.js:116:37)
    at Object.<anonymous> (/home/earenas/projects/my_project/node_modules/xliff-simple-merge/dist/src/index.js:29:37)

Node.js v18.15.0

The sax library error indicates the exclude file starts with the invalid character p instead of starting by a proper XML tag. However, if I change the --exclude-file option to aaa.xlf it shows the same error but telling that a is not a valid start (the file aaa.xlf doesn't even exist).

Workaround

Passing the actual content works:

npx xliff-simple-merge \
    -i projects/user-console/src/locale/messages.xlf \
    -e "$(cat projects/shared/src/locale/messages.xlf)" \
    -d output.xlf

Possible solution

I debugged the execution and found that the mergeWithMapping function expects options.excludeFiles to be the XML content as it happens with the inFilesContent argument:

export function mergeWithMapping(inFilesContent: string | string[], destFileContent: string, options?: MergeOptions, destFilePath?: string): [mergedDestFileContent: string, idMappging: { [oldId: string]: string }] {
    inFilesContent = Array.isArray(inFilesContent) ? inFilesContent : [inFilesContent];
    const inDocs = inFilesContent.map(inFileContent => new XmlDocument(inFileContent)); // <-------------------- XmlDocument uses the inFileContent
    const xliffVersion = inDocs[0].attr.version as '1.2' | '2.0' ?? '1.2';

    const destDoc = new XmlDocument(destFileContent.match(/^[\n\r\s]*$/) ? createEmptyTarget(xliffVersion === '2.0', extractSourceLocale(inDocs[0], xliffVersion === '2.0'), extractTargetLocale(destFilePath)) : destFileContent);
    const excludeDocs = (options?.excludeFiles ?? []).map(excludeFile => new XmlDocument(excludeFile));  // <-------------------- BUT this excludeFile is not the content but the path!!

    const destUnitsParent = xliffVersion === '2.0' ? destDoc.childNamed('file')! : destDoc.childNamed('file')?.childNamed('body')!;
    const excludeUnits = excludeDocs.map(excludeDoc => getUnits(excludeDoc, xliffVersion) ?? []).flat(1);
    const excludeUnitsId = new Set<string>(excludeUnits.map(unit => unit.attr.id!));
    const inUnits = inDocs.map(inDoc => getUnits(inDoc, xliffVersion) ?? []).flat(1).filter(inUnit => !excludeUnitsId .has(inUnit.attr.id));
   ...

I suppose that the fix should be loading the file at the index.ts as you do with the inFilesContent variable.

Please, let me know if I should provide more information or any kind support.

daniel-sc commented 3 months ago

@emarbo Thanks for this detailed issue. It looks like your analysis is correct. Would you like to make a PR? 😊

emarbo commented 3 months ago

Yes, of course. I'll pushing the PR at some time during this week :raised_hands:

daniel-sc / xliff-simple-merge