Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for ISO/IEC 19757-3:2016, Section 5.4.4, <include /> tag #28

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

esvit
Copy link
Contributor

@esvit esvit commented Jan 4, 2025

No description provided.

@wvbe
Copy link
Owner

wvbe commented Jan 4, 2025

Hey @esvit,

Let me start by saying massive thanks for your PR. Code is super clear, love the naming too, no further questions.

I am seeing only two things that I think should be improved before integration, and I'm hoping to commit a suggestion back to your PR to solve them:

  1. Trimming strings should not be node-schematron's responsibility

Do you have whitespace in your schematron that shouldn't be there? Ideally you take that out, eg. by using an XSLT. I'm treating all whitespace as significant, but this could be a mistake.

  1. node-schematron should not depend on NodeJS fs module, because it is not available in the browser.

What would improve this, and would make it possible for any users to improve the file reference resolution, is if resourceDir was replaced with a retrieveFile option of sorts. If it accepts two parameters referencedFile: string and referencingFile: string, you could use it to resolve relative file references, UUID references (eg. in combination with catalog.xml), and things like that. So long as the function returns string | Promise<string> XML, then the option can be implemented with fs.readFile or fs.readFileSync, fetch, or other kinds of lookups.

Are you at the moment blocked on the release of support for <include />?

Best,

  • Wybe

@wvbe
Copy link
Owner

wvbe commented Jan 4, 2025

@esvit I pushed an additional commit to your fork to fix the issues I mentioned in my previous comment. Please see ad8113e

Here's the whole list of remarks on my own commit:

  • Replaced the resourceDir: string option with fetchReference: (href: string, referenceChain: string[]) => string | Promise<string>. This option is a little more complicated than I thought initially, but it gives all the flexibility it needs: an implementation can choose how to resolve references (absolute, relative, or some other), and can choose how to return the XML string (sync or async). I've added a new test to your suite that demonstrates this.
  • The test suite now asserts that included files can include more files, recursively.
  • I've added overloads to Schema.fromString, Schema.fromDom and Schema.fromDomToJson so that this would not have to be a breaking change. Someone now using the option to resolve <include> will still synchronously get the Schema instance back -- but somebody who does will always get a Promise<Schema>, which vibes well with the good chance that reading an included file is an async operation.
  • Replaced slimdom-sax-parser with simply slimdom. This is unrelated to the feature, but since I was fixing a typing thing it was opportune to do it now. Slimdom has implemented parseXmlDocument a few years ago, making the slimdom-sax-parser lib redundant for the purposes of node-schematron.

I've published this to the npm registry as [email protected], hopefully this makes it easy for you to try out. Please let me know your thoughts, and if you agree, I'll finish up on squash/merge/publish again :)

Many thanks

  • Wybe

@wvbe wvbe merged commit a0c7745 into wvbe:master Jan 21, 2025
@wvbe
Copy link
Owner

wvbe commented Jan 21, 2025

@esvit I've merged and published your change with my own commit on top if it, as v2.1.0. Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants