lowlighter / libs

🍱 Collection of carefully crafted TypeScript standalone libraries. Minimal, unbloated, convenient.
https://jsr.io/@libs
MIT License
125 stars 11 forks source link

Provide a migration guide of XML library from v4 to v5 #56

Closed oscarotero closed 6 months ago

oscarotero commented 6 months ago

Hi. Thanks for these great libraries, specially XML.

I've been using it for some Lume plugins like this one to generate Feeds. I tried to upgrade to v5 but couldn't find any documentation or examples about how to do it. The only documentation is JSR docs page, which doesn't help much.

I have been looking the examples in the test files, but I got this error that I don't know how to fix.

 Argument of type '{ "@version": string; "@encoding": string; rss: { "@xmlns:content": string; "@xmlns:wfw": string; "@xmlns:dc": string; "@xmlns:atom": string; "@xmlns:sy": string; "@xmlns:slash": string; "@version": string; channel: { ...; }; }; }' is not assignable to parameter of type 'Partial<xml_document>'.
  Types of property '"@version"' are incompatible.
    Type 'string' is not assignable to type '`1.${number}`'.
lowlighter commented 6 months ago

Hi ! Thanks a lot for using these !

I'll write a migration guide later today or early this week 👍

In the meantime, looking at your example I think you can do:

  {
-     xml: {
-       "@version": "1.0",
-       "@encoding": "UTF-8",
-     },
+    "@version": "1.0" as const, // or as `1.${number}`
+    "@encoding": "UTF-8",
  }

The error you're seeing is because I tested enforcing the validity of 1.x of xml spec with the typing but I guess it's not really practical since you need to almost always cast it back. I'll probably change it back to string in a patch to make it less troublesome

And for $XML this has been removed

API changes:

  • Parent node is now stored in "~parent" non-enumerable property
  • Tag name is now stored in "~name" non-enumerable property
  • Children nodes are now stored in "~children" non-enumerable property
  • Comments texts are now stored in "#comments" property
  • XML doctype is now stored in "#doctype" property
  • XML processing instructions are now stored in "#instructions" property

~parent and ~name are supposed to be irrevelant in stringify(), however because of this it mishandle the way text nodes are working which means ~cdata and ~comment are currently not handled correctly in v5, so I'll also provide a patch really soon, but basically the expected change would be something along:

{
  "~name": "~cdata",
  "#text": "content"
}

But I think i'll provide utility functions in like cdata(text: string) and comment(text: string) as helpers to create these structs to make it more clean and verbose

So for now unless you don't really have the need to use CDATA, I'd advise staying on previous version.

Also not sure if you use XML.parse() but one major change is that mixed content is now parsed (you can either get the #text or child nodes individually) but this definitely breaks what was previously returned

oscarotero commented 6 months ago

Great, thanks for the fast and detailed explanation!

So for now unless you don't really have the need to use CDATA, I'd advise staying on previous version.

Sure, I'll wait for your patches. No rush.

I also see that you are planning to publish it on land/x again. I'd love that (I prefer HTTP imports over than JSR imports if it's possible). I tested the v5 on land/x but got some errors due the existing of bare imports in the code. I guess you're aware of that, but I let you know just in case.

lowlighter commented 6 months ago

@oscarotero I've published the migration guide, let me know if there are still unclear things or if you are still encountering issues. I've also patched the discussed bugs (typing for xml version and cdata/comment handling).

Looking at your usecase again I think it'll should be fairly easy to migrate. In addition to the changes already mentioned above (about moving xml properties to top object and removing $XML), you'll just have to import and call cdata() on item.content.

And I think I finally got my cross-runtime cross-registries setup working, so now this package is published on jsr.io, npm and also deno.land/x so you can import it via https. I've tested the latter (https://deno.land/x/xml@5.4.2) locally against the test suite and it works as expected

Thanks again for using this lib in a project as cool as lume !

oscarotero commented 6 months ago

@lowlighter Thank you for creating the migration guide so fast! 🙏 I'm goint to upgrade it and will let you know if I have problems.

About the land/x publishing, I'm afraid it doesn't work because you're using bare imports in the code (land/x doesn't replace the imports in the code with the import maps entries).

error: Failed resolving types. Relative import path "@libs/typing" not prefixed with / or ./ or ../ and not in import map from "https://deno.land/x/xml@5.4.2/parse.ts"
    at https://deno.land/x/xml@5.4.2/parse.ts:3:43

A possible solution is to use directly the jsr specifier instead of import maps in the code. for example:

- import type { Nullable, record, rw } from "@libs/typing";
+ import type { Nullable, record, rw } from "jsr:@libs/typing@2";
lowlighter commented 6 months ago

@oscarotero After doing some shenanigans https://deno.land/x/xml@5.4.6 should be usable 👍 Sorry when I tested I forgot there was still the import maps of @libs 😅

I can't directly use specifiers because I rely on imports maps for cross-runtime testing (node/bun doesn't support jsr:/npm: prefixes, and it's troublesome to use custom loaders on them deno is the only one that can do it). Since I didn't want to maintain two versions of the same codebase, I just created a small tool to "unmap" imports which creates a temporary git branch with all imports resolved to the content of the import map and which gets published to deno.land/x. The whole process for releasing and publishing is automated, so you can use the import the package from whichever source you'd prefer

oscarotero commented 6 months ago

Wow, thanks for all your work, specially for creating the unmap tool to deal with Deno limitations! I confirm it works great.

This makes me think of a publishing tool to automatically convert and publish code for all runtimes (something like DNT but with more features). It's just an idea :)

Thanks again!

lishaduck commented 5 months ago

In the meantime, looking at your example I think you can do:

  {
-     xml: {
-       "@version": "1.0",
-       "@encoding": "UTF-8",
-     },
+    "@version": "1.0" as const, // or as `1.${number}`
+    "@encoding": "UTF-8",
  }

The error you're seeing is because I tested enforcing the validity of 1.x of xml spec with the typing but I guess it's not really practical since you need to almost always cast it back. I'll probably change it back to string in a patch to make it less troublesome

Without having looked closely at the source code, couldn't you use use const type parameters? The types probably wouldn't be as pretty, but I think it would fix it. I've used them to much success.

lowlighter commented 5 months ago

Without having looked closely at the source code, couldn't you use use const type parameters? The types probably wouldn't be as pretty, but I think it would fix it. I've used them to much success.

Well although I think there's only "1.0" and "1.1" that are used, the spec technically allow for "1.x" as they define it this way:

VersionNum ::= '1.' [0-9]+

Does what you mentioned supports const T extends \1.${string}`` or it only for literals ? (can't check right now)

lishaduck commented 5 months ago

Does what you mentioned supports const T extends \1.${string}`` or it only for literals ? (can't check right now)

That's what it's for. The idea is that you can make generics that automatically as const themselves. The only issue would be that it's for functions, while this is an object property. I would think you could do T extends Obj, with obj being the xml_document type, or whatnot. It would definitely work for a literal type, the only question is if it would work for a property of an object. I would assume so, but I haven't used it that much.

lishaduck commented 5 months ago

Well, yeah you could. If you make xml_document (or whatever the exported type is) generic, make it have T extends `1.${string}`, then use that as the type param in the function, then you could type parametrize the obj, not the whole param. I'll see if I can get it to work later today if I have time.

lishaduck commented 5 months ago

Is there a reason that the type was `1.${string}`, rather than `1.${number}`?

lishaduck commented 5 months ago

Ok, I think I got something working. It should just be the same as before, but with some generics. I'm not seeing any type errors in the tests though. Is there a good way to test that it infers right?

lishaduck commented 5 months ago

nvm.

const _example = stringify({
  "@version": "1.1",
  "@encoding": "UTF-8",
  "@standalone": "yes",
  "#doctype": {
    "~name": "html",
    "~children": [],
    "#text": "",
  },
  "#instructions": {},
  "~name": "html",
  "~children": [],
  "#text": "",
})

The generic is inferred correctly without needing to type it! I'll go send a PR.