chore(common): xml2js -> fast-xml-parser #12331

srl295 · 2024-08-30T02:11:59Z

replace xml2js with fast-xml-parser (much better support)

Fixes: #12208

@keymanapp-test-bot skip

- just the 'easy stuff' in this commit

- it compiles, but broken

…string

- update numbers- skip all numbers

- reverted to fontsize being a string - updated the kvks schema to match current xml

- assert.doesNotThrow just truncates the exception with no benefit - assert early that there are no messages when checking for a load failure

Fixes: #12208

keymanapp-test-bot · 2024-08-30T02:12:03Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 18.0.115 (0.12331.12061)
Web
- KeymanWeb Test Home
Windows

common/schemas/kvks/kvks.schema.json

developer/src/common/web/utils/src/types/kpj/kpj-file-reader.ts

developer/src/common/web/utils/package-lock.json

mcdurdin · 2024-08-30T04:20:44Z

@SabineSIL FYI

Fixes: #12208

- need to use trimValues:false and a custom tagValueProcessor to preserve spaces Fixes: #12208

srl295 · 2024-09-09T22:17:25Z

Making some good progress, it looks like the PUA issue has to do with NCR's, either not being par or being skipped if there's an error

- difference in fast-xml-parser and xml2js handling here - part of the move to fast-xml-parser Fixes: #12208

The visual keyboard compiler was never finished in 17.0. This rewrites it to: 1. Use the kmxplus data rather than reading from xml directly 2. Fill in `visualkeyboard.header.kbdname` 3. Support modifiers 4. Handle encoded characters like `\u{1234}` 5. Handle string variables like `${one}`* Additional unit tests have been added to verify the behavior of the visual keyboard compiler in more detail. TODO-LDML: string variables appear to have a secondary bug -- they seem to be returning the string 'undefined'. I have disabled the related tests and will examine this separately, and enable those tests once fixed. TODO-LDML: we should probably add a compiler warning + unit test for `<layers formId="us"><layer id="base">`, because this pattern does not make sense: when using non-touch forms, the `<layer>` element should use `modifiers` attribute, and correspondingly, `modifiers` attribute should _not_ be used when `formId` is `touch`. Other fixes: 1. The LDML XML reader was relying on its input being a Node.js `Buffer` even though it was declared `Uint8Array`, as it implicitly used `Buffer.toString()` to do text conversion. (`Buffer` subclasses from `Uint8Array`). This breaks when using `Uint8Array` directly and means we had an implicit dependency on Node.js. See also #12331. 2. XML errors were not captured in the LDML XML reader. See also #12331. 3. The unused and unfinished touch-layout-compiler.ts and keymanweb-compiler.ts have been removed along with corresponding unit tests and fixtures. These are replaced by Core implementations; see #12291. Fixes: #12395

The visual keyboard compiler was never finished in 17.0. This rewrites it to: 1. Use the kmxplus data rather than reading from xml directly 2. Fill in `visualkeyboard.header.kbdname` 3. Support modifiers 4. Handle encoded characters like `\u{1234}` 5. Handle string variables like `${one}`* Additional unit tests have been added to verify the behavior of the visual keyboard compiler in more detail. * String variable tests will be enabled in next commit (which is a cherry-pick of #12404). Other fixes: 1. The LDML XML reader was relying on its input being a Node.js `Buffer` even though it was declared `Uint8Array`, as it implicitly used `Buffer.toString()` to do text conversion. (`Buffer` subclasses from `Uint8Array`). This breaks when using `Uint8Array` directly and means we had an implicit dependency on Node.js. See also #12331. 2. XML errors were not captured in the LDML XML reader. See also #12331. 3. The unused and unfinished touch-layout-compiler.ts and keymanweb-compiler.ts have been removed along with corresponding unit tests and fixtures. These will be replaced by Core implementations; see #12291. Fixes: #12395 Cherry-pick-of: #12402

…8/fastxmlparser

Fixes: #12208

turns out we didn't have a test for XML NCR in keyboards. Now we do. Fixes: #12208

srl295 · 2024-09-12T20:15:59Z

I'd like to refactor the common parsing, etc as a followon PR after we make sure this is actually working due to the dependency change.

mcdurdin

I've started the review but have a significant change request which will, I hope, reduce the impact and scope. As it stands, I think the changes would have a number of negative cascading impacts on consumers of the data, and so I would prefer to have the data coming out of the parser match the current shape, so we don't have to change anything downstream from the parser.

mcdurdin · 2024-09-13T01:32:10Z

common/web/types/src/util/util.ts

@@ -2,7 +2,9 @@ import { MATCH_HEX_ESCAPE, CONTAINS_QUAD_ESCAPE, MATCH_QUAD_ESCAPE } from './con
 export { MATCH_HEX_ESCAPE, CONTAINS_QUAD_ESCAPE, MATCH_QUAD_ESCAPE };

 /**
- * xml2js will not place single-entry objects into arrays. Easiest way to fix
+ * fast-xml-parser will not place single-entry objects into arrays.
+ * We could set a list of items expected to be arrays,


Is this declarative approach viable? It seems like it would be cleaner

mcdurdin · 2024-09-13T01:33:17Z

developer/src/common/web/utils/src/common-events.ts

+    // may or may not have extended details
+    if (o.code && o.line && o.msg) {
+      return m(this.ERROR_InvalidXML,
+        `Error reading XML at ${def(o.line)}:${def(o.col)}: ${def(o.code)} ${def(o.msg)}`


Suggested change

`Error reading XML at ${def(o.line)}:${def(o.col)}: ${def(o.code)} ${def(o.msg)}`

`Error reading XML at ${def(o.line)}${o.col ? ':' + def(o.col) : ''}: ${def(o.code)} ${def(o.msg)}`

This is starting to get a little too 'formatted' for translators so we need to be careful

mcdurdin · 2024-09-13T01:38:47Z

web/src/engine/common/web-utils/package.json

+  "dependencies": {
+    "fast-xml-parser": "^4.4.1"
+  }


This shouldn't be in web-utils deps. web-utils is not part of Keyman Developer. It is a part of the KeymanWeb source base (see #12384 for its move to /web).

fast-xml-parser needs to be in package.json for:

developer/src/common/web/utils

developer/src/kmc-ldml

developer/src/kmc-package

mcdurdin · 2024-09-13T01:43:54Z

developer/src/common/web/utils/src/types/kpj/kpj-file-reader.ts

-      data = r as KPJFile;
-    });
+  const parser = new XMLParser({
+    htmlEntities: true,


We are not expecting html entities in a .kpj file -- it is pure XML

Suggested change

htmlEntities: true,

htmlEntities: false,

mcdurdin · 2024-09-13T01:44:32Z

developer/src/common/web/utils/src/types/kpj/kpj-file-reader.ts

+  const parser = new XMLParser({
+    htmlEntities: true,
+    ignoreAttributes: false, // We'd like attributes, please
+    attributeNamePrefix: '', // to avoid '@_' prefixes


I don't think it's a good idea to mix attributes and children, can we use a '$' prefix for attributes? (same goes for .kvks)

See later comment for bigger request that makes this suggestion null

mcdurdin · 2024-09-13T01:48:27Z

developer/src/common/web/utils/src/types/kpj/kpj-file-reader.ts

+    ignoreAttributes: false, // We'd like attributes, please
+    attributeNamePrefix: '', // to avoid '@_' prefixes
+    numberParseOptions: {
+      skipLike: /(?:)/, // parse numbers as strings


I take it this somewhat awkward pattern leaves numbers as strings? Shame FXP doesn't have a numberParse: false option!

mcdurdin · 2024-09-13T01:49:36Z

developer/src/common/web/utils/src/types/kpj/kpj-file-reader.ts

+    },
+  });
+    const raw = parser.parse(file.toString());
+    delete raw['?xml']; // XML prologue


does option ignoreDeclaration work?

mcdurdin · 2024-09-13T01:54:46Z

developer/src/common/web/utils/src/types/kps/kps-file.ts

-  _: string;
-  $: { URL: string };
+  '#text': string;
+  URL: string;


This is an API change which I am not excited about -- I would prefer to keep the same interfaces where possible. Yes, the original interface does inherit the shape of xml2js, but it's what we have now. We don't know if there are other consumers outside the code base. Even internally, you've missed, for example:

keyman/developer/src/kmc-package/src/compiler/windows-package-installer-compiler.ts

Line 232 in e67aed1

setupInf += `${str.$?.Name}=${str.$?.Value}\n`;

(kpj.schema.json would also need updating if we continue with this change)

Does this work to match existing shape?

attributesGroupName: '$', attributesNamePrefix: '', ignoreAttributes: false, textNodeName: '_',

If we do go this way, we don't need to update any schema.json files, nor any consumers of the XML-as-js, AFAICT.

srl295 · 2024-09-13T02:18:44Z

I've started the review but have a significant change request which will, I hope, reduce the impact and scope. As it stands, I think the changes would have a number of negative cascading impacts on consumers of the data, and so I would prefer to have the data coming out of the parser match the current shape, so we don't have to change anything downstream from the parser.

Some seem to be positive changes, but make some sense, basically xml2js: true mode.

Yeah I think I can do it. Not this sprint or at least not this day though

mcdurdin · 2024-09-13T02:22:59Z

Some seem to be positive changes

Oh for sure, but negative in that they require lots of changes to consumers of the API, which is costly.

…fastxmlparser

ermshiperete · 2024-09-18T06:42:33Z

developer/src/common/web/utils/src/types/kps/kps-file.ts

+  /**
+   * relationship between this package and the related package, "related" is default if not specified
+   */
+  Relationship?: "deprecates";


Shouldn't this be

Suggested change

Relationship?: "deprecates";

Relationship?: "deprecates"|"related";

if the default is "related"? (I didn't check if there are other possible values)

Currently only "deprecates" is allowed:

keyman/common/schemas/kps/kps.xsd

Lines 108 to 109 in 4004449



<xs:attribute name="Relationship" type="km-package-relationship" />

Ah, ok. The comment in kps.xsd makes that much clearer.

ermshiperete · 2024-09-20T13:16:58Z

developer/src/common/web/utils/src/types/kps/kps-file.ts

+  /**
+   * relationship between this package and the related package, "related" is default if not specified
+   */
+  Relationship?: "deprecates";


Suggested change

/**

* relationship between this package and the related package, "related" is default if not specified

*/

Relationship?: "deprecates";

/**

* relationship between this package and the related package, if not specified it means "related"

*/

Relationship?: "deprecates";

ermshiperete · 2024-09-20T13:18:29Z

developer/src/common/web/utils/src/types/kps/kps-file.ts

+  /**
+   * relationship between this package and the related package, "related" is default if not specified
+   */
+  Relationship?: "deprecates";


Ah, ok. The comment in kps.xsd makes that much clearer.

mcdurdin · 2024-09-28T00:49:36Z

did you mean to close this?

srl295 · 2024-09-28T02:47:12Z

did you mean to close this?

Yes, in favor of one based on #12482 (I guess I could reopen this once sorted)

mcdurdin · 2024-09-28T02:49:16Z

did you mean to close this?

Yes, in favor of one based on #12482 (I guess I could reopen this once sorted)

OK, np. Please make sure you document when you do actions like this because I was quite confused!

Shall I'll go ahead and delete the branch?

srl295 · 2024-09-28T03:39:13Z

Shall I'll go ahead and delete the branch?

Not yet because I am using it to work from!

I will either reopen it or open a new one

srl295 added 9 commits August 29, 2024 12:26

chore(common): xml2js -> fast-xml-parser

ca848cb

- just the 'easy stuff' in this commit

chore(common): xml2js -> fast-xml-parser first pass at implementation

3bf7ff2

- it compiles, but broken

chore(common): fast-xml-parser: kbd and test data loads

7bc8747

chore(common): fast-xml-parser: delving into kpj and kvks

1086b3f

chore(common): fast-xml-parser: fix test inconsistencies with number/…

b044396

…string

chore(common): fast-xml-parser: kvks reader

419a752

- update numbers- skip all numbers

chore(common): kvks: updated schema

06672d5

- reverted to fontsize being a string - updated the kvks schema to match current xml

chore(developer): improve test messages messages

067ad73

- assert.doesNotThrow just truncates the exception with no benefit - assert early that there are no messages when checking for a load failure

chore(developer): improve kpj schema

030e1b6

Fixes: #12208

keymanapp-test-bot bot added this to the A18S9 milestone Aug 30, 2024

github-actions bot added developer/ common/ common/web/ chore labels Aug 30, 2024

mcdurdin reviewed Aug 30, 2024

View reviewed changes

common/schemas/kvks/kvks.schema.json Outdated Show resolved Hide resolved

mcdurdin reviewed Aug 30, 2024

View reviewed changes

developer/src/common/web/utils/src/types/kpj/kpj-file-reader.ts Outdated Show resolved Hide resolved

mcdurdin reviewed Aug 30, 2024

View reviewed changes

developer/src/common/web/utils/package-lock.json Outdated Show resolved Hide resolved

darcywong00 modified the milestones: A18S9, A18S10 Aug 31, 2024

srl295 added 2 commits August 31, 2024 08:41

chore(developer): Drop stray package-lock.json

8779ca9

Fixes: #12208

chore(developer): update kps/kmp parsing

a1f1bf7

Fixes: #12208

github-actions bot added the developer/compilers/ label Aug 31, 2024

srl295 added 4 commits August 31, 2024 20:35

chore(developer): kmc-package working

877522c

Fixes: #12208

chore(developer): add invalid XML error

1cb33da

Fixes: #12208

chore(developer): unboxing fix for displays

06a5683

Fixes: #12208

chore(developer): fix for overly-trimmed attributes

171e37c

- need to use trimValues:false and a custom tagValueProcessor to preserve spaces Fixes: #12208

chore(common): handle an empty <transformGroup/> properly

c24ba0f

- difference in fast-xml-parser and xml2js handling here - part of the move to fast-xml-parser Fixes: #12208

mcdurdin mentioned this pull request Sep 11, 2024

fix(developer): rewrite ldml visual keyboard compiler #12402

Merged

mcdurdin mentioned this pull request Sep 12, 2024

fix(developer): rewrite ldml visual keyboard compiler 🍒 🏠 #12406

Merged

Merge remote-tracking branch 'upstream/master' into chore/common/1220…

f11e77a

…8/fastxmlparser

github-actions bot added the web/ label Sep 12, 2024

srl295 added 2 commits September 12, 2024 14:26

chore(developer): fix for kvks writing

02330e1

Fixes: #12208

chore(developer): fix for NCR parsing

6f7f268

turns out we didn't have a test for XML NCR in keyboards. Now we do. Fixes: #12208

srl295 self-assigned this Sep 12, 2024

srl295 marked this pull request as ready for review September 12, 2024 20:30

srl295 requested review from ermshiperete, jahorton and darcywong00 as code owners September 12, 2024 20:30

mcdurdin requested changes Sep 13, 2024

View reviewed changes

darcywong00 modified the milestones: A18S10, A18S11 Sep 14, 2024

Merge remote-tracking branch 'origin/master' into chore/common/12208/…

d4a3407

…fastxmlparser

ermshiperete reviewed Sep 18, 2024

View reviewed changes

ermshiperete reviewed Sep 20, 2024

View reviewed changes

srl295 closed this Sep 27, 2024

srl295 mentioned this pull request Sep 28, 2024

feat(common): unified XML parser/writer #12482

Merged

srl295 mentioned this pull request Oct 3, 2024

chore(developer,common): deps: replace xml2js with fast-xml-parser #12502

Merged

srl295 deleted the chore/common/12208/fastxmlparser branch October 25, 2024 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(common): xml2js -> fast-xml-parser #12331

chore(common): xml2js -> fast-xml-parser #12331

srl295 commented Aug 30, 2024

keymanapp-test-bot bot commented Aug 30, 2024 •

edited

Loading

mcdurdin commented Aug 30, 2024

srl295 commented Sep 9, 2024

srl295 commented Sep 12, 2024

mcdurdin left a comment

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

mcdurdin Sep 13, 2024

srl295 commented Sep 13, 2024 •

edited

Loading

mcdurdin commented Sep 13, 2024

ermshiperete Sep 18, 2024

mcdurdin Sep 19, 2024

ermshiperete Sep 20, 2024

ermshiperete Sep 20, 2024

ermshiperete Sep 20, 2024

mcdurdin commented Sep 28, 2024

srl295 commented Sep 28, 2024

mcdurdin commented Sep 28, 2024

srl295 commented Sep 28, 2024

	`Error reading XML at ${def(o.line)}:${def(o.col)}: ${def(o.code)} ${def(o.msg)}`
	`Error reading XML at ${def(o.line)}${o.col ? ':' + def(o.col) : ''}: ${def(o.code)} ${def(o.msg)}`

	Relationship?: "deprecates";
	Relationship?: "deprecates"\|"related";

	<!-- "deprecates" is only valid value, if not present then means "related" -->
	<xs:attribute name="Relationship" type="km-package-relationship" />

chore(common): xml2js -> fast-xml-parser #12331

chore(common): xml2js -> fast-xml-parser #12331

Conversation

srl295 commented Aug 30, 2024

keymanapp-test-bot bot commented Aug 30, 2024 • edited Loading

User Test Results

Test Artifacts

mcdurdin commented Aug 30, 2024

srl295 commented Sep 9, 2024

srl295 commented Sep 12, 2024

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Sep 13, 2024 • edited Loading

mcdurdin commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdurdin commented Sep 28, 2024

srl295 commented Sep 28, 2024

mcdurdin commented Sep 28, 2024

srl295 commented Sep 28, 2024

keymanapp-test-bot bot commented Aug 30, 2024 •

edited

Loading

srl295 commented Sep 13, 2024 •

edited

Loading