Skip to content

Commit

Permalink
feat(developer): update the unified xml parser per review
Browse files Browse the repository at this point in the history 
- declarative syntax for options
- workaround an issue where xml2js is mutating our options objects
- improve tests

Fixes: #12208
  • Loading branch information
@srl295
srl295 committed Sep 30, 2024
1 parent eba7079 commit 158c4a7
Show file tree
Hide file tree
Showing 8 changed files with 113 additions and 115 deletions.
4 changes: 1 addition & 3 deletions developer/src/common/web/utils/src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,4 @@ export { defaultCompilerOptions, CompilerBaseOptions, CompilerCallbacks, Compile

export { CommonTypesMessages } from './common-messages.js';

export * as xml2js from './deps/xml2js/xml2js.js';

export { KeymanXMLOptions, KeymanXMLWriter, KeymanXMLReader } from './xml-utils.js';
export { KeymanXMLType, KeymanXMLWriter, KeymanXMLReader } from './xml-utils.js';
2 changes: 1 addition & 1 deletion developer/src/common/web/utils/src/types/kpj/kpj-file-reader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export class KPJFileReader {
public read(file: Uint8Array): KPJFile {
let data: KPJFile;

data = new KeymanXMLReader({ type: 'kpj' })
data = new KeymanXMLReader('kpj')
.parse(file.toString());

data = this.boxArrays(data);
Expand Down
2 changes: 1 addition & 1 deletion developer/src/common/web/utils/src/types/kvks/kvks-file-reader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default class KVKSFileReader {
let source: KVKSourceFile;

try {
source = new KeymanXMLReader({ type: 'kvks' })
source = new KeymanXMLReader('kvks')
.parse(file.toString()) as KVKSourceFile;
} catch(e) {
if(file.byteLength > 4 && file.subarray(0,3).every((v,i) => v == KVK_HEADER_IDENTIFIER_BYTES[i])) {
Expand Down
2 changes: 1 addition & 1 deletion developer/src/common/web/utils/src/types/kvks/kvks-file-writer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export default class KVKSFileWriter {
l.key.push(k);
}

const result = new KeymanXMLWriter({type: 'kvks'}).write(kvks);
const result = new KeymanXMLWriter('kvks').write(kvks);
return result; //Uint8Array.from(result);
}

Expand Down
4 changes: 2 additions & 2 deletions developer/src/common/web/utils/src/types/ldml-keyboard/ldml-keyboard-xml-reader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ export class LDMLKeyboardXMLSourceFileReader {

loadUnboxed(file: Uint8Array): LDMLKeyboardXMLSourceFile {
const data = new TextDecoder().decode(file);
const source = new KeymanXMLReader({ type: 'keyboard3' })
const source = new KeymanXMLReader('keyboard3')
.parse(data) as LDMLKeyboardXMLSourceFile;
return source;
}
Expand Down Expand Up @@ -293,7 +293,7 @@ export class LDMLKeyboardXMLSourceFileReader {
}

loadTestDataUnboxed(file: Uint8Array): any {
const source = new KeymanXMLReader({ type: 'keyboard3-test' })
const source = new KeymanXMLReader('keyboardTest3')
.parse(file.toString()) as any;
return source;
}
Expand Down
180 changes: 89 additions & 91 deletions developer/src/common/web/utils/src/xml-utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,84 @@
* Abstraction for XML reading and writing
*/

import { xml2js } from "./index.js";
import * as xml2js from "./deps/xml2js/xml2js.js";

export class KeymanXMLOptions {
type: 'keyboard3' // LDML <keyboard3>
| 'keyboard3-test' // LDML <keyboardTest3>
| 'kps' // <Package>
| 'kvks' // <visualkeyboard>
| 'kpj' // // <KeymanDeveloperProject>
;
}
export type KeymanXMLType =
'keyboard3' // LDML <keyboard3>
| 'keyboardTest3' // LDML <keyboardTest3>
| 'kps' // <Package>
| 'kvks' // <visualkeyboard>
| 'kpj' // <KeymanDeveloperProject>
;

/** Bag of options, maximally one for each KeymanXMLType */
type KemanXMLOptionsBag = {
[key in KeymanXMLType]?: any
};

/** map of options for the XML parser */
const PARSER_OPTIONS: KemanXMLOptionsBag = {
'keyboard3': {
explicitArray: false,
mergeAttrs: true,
includeWhiteChars: false,
emptyTag: {} as any
// Why "as any"? xml2js is broken:
// https://github.com/Leonidas-from-XIV/node-xml2js/issues/648 means
// that an old version of `emptyTag` is used which doesn't support
// functions, but DefinitelyTyped is requiring use of function or a
// string. See also notes at
// https://github.com/DefinitelyTyped/DefinitelyTyped/pull/59259#issuecomment-1254405470
// An alternative fix would be to pull xml2js directly from github
// rather than using the version tagged on npmjs.com.
},
'keyboardTest3': {
preserveChildrenOrder: true, // needed for test data
explicitChildren: true, // needed for test data
},
'kps': {
explicitArray: false
},
'kpj': {
explicitArray: false,
mergeAttrs: false,
includeWhiteChars: false,
normalize: false,
emptyTag: ''
},
'kvks': {
explicitArray: false,
mergeAttrs: false,
includeWhiteChars: true,
normalize: false,
emptyTag: {} as any
// Why "as any"? xml2js is broken:
// https://github.com/Leonidas-from-XIV/node-xml2js/issues/648 means
// that an old version of `emptyTag` is used which doesn't support
// functions, but DefinitelyTyped is requiring use of function or a
// string. See also notes at
// https://github.com/DefinitelyTyped/DefinitelyTyped/pull/59259#issuecomment-1254405470
// An alternative fix would be to pull xml2js directly from github
// rather than using the version tagged on npmjs.com.
},
};

const GENERATOR_OPTIONS: KemanXMLOptionsBag = {
kvks: {
allowSurrogateChars: true,
attrkey: '$',
charkey: '_',
xmldec: {
version: '1.0',
encoding: 'UTF-8',
standalone: true
},
},
};

/** wrapper for XML parsing support */
export class KeymanXMLReader {
public constructor(public options: KeymanXMLOptions) {
public constructor(public type: KeymanXMLType) {
}

public parse(data: string): any {
Expand All @@ -30,72 +94,16 @@ export class KeymanXMLReader {
}

public parser() {
const { type } = this.options;
switch (type) {
case 'keyboard3':
return new xml2js.Parser({
explicitArray: false,
mergeAttrs: true,
includeWhiteChars: false,
emptyTag: {} as any
// Why "as any"? xml2js is broken:
// https://github.com/Leonidas-from-XIV/node-xml2js/issues/648 means
// that an old version of `emptyTag` is used which doesn't support
// functions, but DefinitelyTyped is requiring use of function or a
// string. See also notes at
// https://github.com/DefinitelyTyped/DefinitelyTyped/pull/59259#issuecomment-1254405470
// An alternative fix would be to pull xml2js directly from github
// rather than using the version tagged on npmjs.com.
});
case 'keyboard3-test':
return new xml2js.Parser({
// explicitArray: false,
preserveChildrenOrder: true, // needed for test data
explicitChildren: true, // needed for test data
// mergeAttrs: true,
// includeWhiteChars: false,
// emptyTag: {} as any
// Why "as any"? xml2js is broken:
// https://github.com/Leonidas-from-XIV/node-xml2js/issues/648 means
// that an old version of `emptyTag` is used which doesn't support
// functions, but DefinitelyTyped is requiring use of function or a
// string. See also notes at
// https://github.com/DefinitelyTyped/DefinitelyTyped/pull/59259#issuecomment-1254405470
// An alternative fix would be to pull xml2js directly from github
// rather than using the version tagged on npmjs.com.
});
case 'kps':
return new xml2js.Parser({
explicitArray: false
});
case 'kpj':
return new xml2js.Parser({
explicitArray: false,
mergeAttrs: false,
includeWhiteChars: false,
normalize: false,
emptyTag: ''
});
case 'kvks':
return new xml2js.Parser({
explicitArray: false,
mergeAttrs: false,
includeWhiteChars: true,
normalize: false,
emptyTag: {} as any
// Why "as any"? xml2js is broken:
// https://github.com/Leonidas-from-XIV/node-xml2js/issues/648 means
// that an old version of `emptyTag` is used which doesn't support
// functions, but DefinitelyTyped is requiring use of function or a
// string. See also notes at
// https://github.com/DefinitelyTyped/DefinitelyTyped/pull/59259#issuecomment-1254405470
// An alternative fix would be to pull xml2js directly from github
// rather than using the version tagged on npmjs.com.
});
default:
/* c8 ignore next 1 */
throw Error(`Internal error: unhandled XML type ${type}`);
let options = PARSER_OPTIONS[this.type];
if (!options) {
/* c8 ignore next 1 */
throw Error(`Internal error: unhandled XML type ${this.type}`);
}
options = Object.assign({}, options); // TODO: xml2js likes to mutate the options here. Shallow clone the object.
if (options.emptyTag) {
options.emptyTag = {}; // TODO: xml2js likes to mutate the options here. Reset it.
}
return new xml2js.Parser(options);
}
}

Expand All @@ -105,26 +113,16 @@ export class KeymanXMLWriter {
const builder = this.builder();
return builder.buildObject(data);
}
constructor(public options: KeymanXMLOptions) {
constructor(public type: KeymanXMLType) {
}

public builder() {
switch (this.options.type) {
case 'kvks':
return new xml2js.Builder({
allowSurrogateChars: true,
attrkey: '$',
charkey: '_',
xmldec: {
version: '1.0',
encoding: 'UTF-8',
standalone: true
}
});
default:
/* c8 ignore next 1 */
throw Error(`Internal error: unhandled XML type ${this.options.type}`);
const options = GENERATOR_OPTIONS[this.type];
if (!options) {
/* c8 ignore next 1 */
throw Error(`Internal error: unhandled XML type ${this.type}`);
}
return new xml2js.Builder(Object.assign({}, options)); // Shallow clone in case the options are mutated.
}
}

32 changes: 17 additions & 15 deletions developer/src/common/web/utils/test/test-xml-utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,20 @@ import { env } from 'node:process';
import { readFileSync, writeFileSync } from 'node:fs';


import { KeymanXMLOptions, KeymanXMLReader, KeymanXMLWriter } from '../src/xml-utils.js';
import { KeymanXMLType, KeymanXMLReader, KeymanXMLWriter } from '../src/xml-utils.js';
import { makePathToFixture } from './helpers/index.js';

// if true, attempt to WRITE the fixtures
const { GEN_XML_FIXTURES } = env;

class Case {
options: KeymanXMLOptions;
type: KeymanXMLType;
paths: string[];
};

const read_cases: Case[] = [
{
options: { type: 'keyboard3' },
type: 'keyboard3',
paths: [
// keyboards
'disp_maximal.xml',
Expand All @@ -34,26 +34,26 @@ const read_cases: Case[] = [
'tran_fail-empty.xml',
],
}, {
options: { type: 'keyboard3-test' },
type: 'keyboardTest3',
paths: [
// keyboard test
'k_020_fr-test.xml',
],
}, {
options: { type: 'kvks' },
type: 'kvks',
paths: [
// kvks
'khmer_angkor.kvks',
],
}, {
options: { type: 'kps' },
type: 'kps',
paths: [
// kps
'test_valid.kps',
// 'error_invalid_package_file.kps',
],
}, {
options: { type: 'kpj' },
type: 'kpj',
paths: [
// kpj
'khmer_angkor.kpj',
Expand All @@ -63,7 +63,7 @@ const read_cases: Case[] = [

const write_cases: Case[] = [
{
options: { type: 'kvks' },
type: 'kvks',
paths: [
// kvks
'khmer_angkor2.kvks', // similar to the 'read case' with the similar name, except for whitespace differences and the prologue
Expand Down Expand Up @@ -93,10 +93,8 @@ function writeJson(path: string, data: any) {

describe(`XML Reader Test ${GEN_XML_FIXTURES && '(update mode!)' || ''}`, () => {
for (const c of read_cases) {
const { options, paths } = c;
describe(`test reading ${JSON.stringify(options)}`, () => {
const reader = new KeymanXMLReader(options);
assert.ok(reader);
const { type, paths } = c;
describe(`test reading ${type}`, () => {
for (const path of paths) {
const xmlPath = makePathToFixture('xml', `${path}`);
const jsonPath = makePathToFixture('xml', `${path}.json`);
Expand All @@ -105,12 +103,16 @@ describe(`XML Reader Test ${GEN_XML_FIXTURES && '(update mode!)' || ''}`, () =>
const xml = readData(xmlPath);
assert.ok(xml, `Could not read ${xmlPath}`);

const reader = new KeymanXMLReader(type);
assert.ok(reader);

// now, parse. subsitute endings for Win
const actual = reader.parse(xml.replace(/\r\n/g, '\n'));
assert.ok(actual, `Parser failed on ${xmlPath}`);

// get the expected
const expect = readJson(jsonPath);

if (GEN_XML_FIXTURES) {
console.log(`GEN_XML_FIXTURES: writing ${jsonPath} from actual`);
writeJson(jsonPath, actual);
Expand All @@ -127,9 +129,9 @@ describe(`XML Reader Test ${GEN_XML_FIXTURES && '(update mode!)' || ''}`, () =>

describe(`XML Writer Test ${GEN_XML_FIXTURES && '(update mode!)' || ''}`, () => {
for (const c of write_cases) {
const { options, paths } = c;
describe(`test writing ${JSON.stringify(options)}`, () => {
const writer = new KeymanXMLWriter(options);
const { type, paths } = c;
describe(`test writing ${type}`, () => {
const writer = new KeymanXMLWriter(type);
assert.ok(writer);
for (const path of paths) {
const jsonPath = makePathToFixture('xml', `${path}.json`);
Expand Down
2 changes: 1 addition & 1 deletion developer/src/kmc-package/src/compiler/kmp-compiler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ export class KmpCompiler implements KeymanCompiler {
let a: KpsFile.KpsPackage;

try {
a = new KeymanXMLReader({ type: 'kps' })
a = new KeymanXMLReader('kps')
.parse(data.toString()) as KpsFile.KpsPackage;
} catch(e) {
this.callbacks.reportMessage(PackageCompilerMessages.Error_InvalidPackageFile({e}));
Expand Down

0 comments on commit 158c4a7

Please sign in to comment.