Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Move libxslt to (optional) peerDependencies, devDependencies #181

Merged

Conversation

eyelidlessness
Copy link
Contributor

Addresses enketo/enketo#760 (though slightly differently). I had meant to address this in #171, but it must have slipped my mind.

As an optional peer dependency, client-only usage will be able to skip installing libxslt entirely. Server transform users (including enketo-core and enketo-express, though core can and should be updated to use the client transform) will need to explicitly install libxslt directly.

This is technically a breaking change, and maybe should be released as semver-major. But I think the impact should be fairly minimal because npm will message about the uninstalled peer dependency, and Node will error if importing libxslt fails.

@lognaturel
Copy link
Contributor

This looks like a good approach to me.

Regarding the decision for semver major vs minor, will Core and Express or their install instructions need to change? I think not, right? If not, minor seems reasonable. @JGreenlee interested in your thoughts on this as well.

@JGreenlee
Copy link
Contributor

Sure, seems like this is basically to the same effect as optionalDependencies. LGTM

@lognaturel
Copy link
Contributor

Chatted briefly with @eyelidlessness about the release plan. We're going to make the Transformer release with this change semver major to reflect the fact that projects that depend directly on it will need to explicitly depend on libxslt if they want to continue using server-side transformation.

Core only uses Transformer for dev and test. We'll have those use client-side transforms so that it doesn't depend on libxslt at all. That has no user-facing impact so can be a patch or whatever.

Express will add an explicit dependency on libxslt when it gets this update so that it can continue doing server-side transforms for the time being. That has no user-facing impact so can be a patch or whatever.

We'll release this now so that @JGreenlee and others can benefit from it but we won't update Core and Express immediately.

@lognaturel lognaturel merged commit e90ee9e into enketo:master Jul 3, 2023
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants