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

Use client-side transform for preview-by-URL #540

Conversation

eyelidlessness
Copy link
Contributor

This is our first step towards migrating to Enketo Transformer's new client build (introduced in enketo/enketo-transformer#171). Preview requests specifying a form query parameter will now fetch the XForm directly, and transformation will be performed client-side.

We expect that this change will affect few if any users outside of ODK's usage, and the intent is to begin dog-fooding the new client-side Transformer build to evaluate how to proceed with expanding its usage.

In the near term, I expect this will be at least a minor performance improvement across the board, because these preview requests will no longer need to go through the backend proxy. Firefox will typically see a greater performance benefit, because its XSLT implementation is faster than all other target environments.

I have verified this PR works with

  • Online form submission
  • Offline form submission
  • Saving offline drafts
  • Loading offline drafts
  • Editing submissions
  • Form preview
  • None of the above

None of the others should be impacted, but I'm happy to manually verify that if requested.

What else has been done to verify that this works as intended?

Extensive testing of client-side transform in enketo/enketo-transformer#171.

Why is this the best possible solution? Were any other approaches considered?

It's possible to move more transformation client side, but we want to start small.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This carries the same risks as enketo/enketo-transformer#171; having been tested extensively, we believe those risks are quite low. It also has the same restrictions as any fetch.

The change does remove the API support for the previous implementation, but that functionality wasn't documented. The only functional change for users is that this change (currently) preemptively loads empty URLs for media, since this preview behavior cannot include media. (I am open to reverting that part of the change, I included it because it reduced the number of errors caught while debugging.)

Do we need any specific form for testing your changes? If so, please attach one.

I tested it in my local ODK Central environment, requesting Central's direct links to XForm definitions.

minify: isProduction,
entryNames: '[name]',
external: ['crypto', 'libxslt'],
format: 'esm',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to call this out in the PR notes, but I can call it out here since it’s also going to cause ESLint failures until I fix the config.

This change will now build for ECMAScript Modules. The reason to make this change now is that it allows the client Transformer to be loaded dynamically only when it’s used. This will have other immediate benefits, by allowing some of the shared code to be loaded concurrently. In the future it’ll allow us to dynamically load quite a bit more, which should significantly reduce load time for many usage scenarios.

This isn’t technically a breaking change, as ESM has been supported in our target environments for quite a while, but it’s worth calling out that this is now a hard requirement rather than just a hypothetical one.

@eyelidlessness eyelidlessness force-pushed the feature/client-side-preview-url-transform branch from 974026c to 9834302 Compare March 23, 2023 00:52
Copy link
Contributor

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the API support for the previous implementation

This references https://github.com/enketo/enketo-express/pull/540/files#diff-0e6c59a8df2414ee19f0d9beabc1ca0da139de21a736c6125a8247d25b85da38L328, right? So it used to be that you could request a URL that would preview a form from a given URL but now you can only build the URL directly?

preemptively loads empty URLs for media

That seems like better behavior to me. What makes it controversial? It has the same eventual result, right?

I have read through this carefully and don't see any serious sources of risk. I'm eager start dogfooding!

@eyelidlessness
Copy link
Contributor Author

This references https://github.com/enketo/enketo-express/pull/540/files#diff-0e6c59a8df2414ee19f0d9beabc1ca0da139de21a736c6125a8247d25b85da38L328, right?

Yep, all of the changes in that module.

So it used to be that you could request a URL that would preview a form from a given URL but now you can only build the URL directly?

I’m not sure I understand the question. But to clarify, it just means you can’t make an API call to transform forms by URL, since that functionality is now client side.

That seems like better behavior to me. What makes it controversial? It has the same eventual result, right?

I agree, I don’t think it’s controversial. It’s just slightly beyond the minimal scope of the change so it felt worth mentioning.

I have read through this carefully and don't see any serious sources of risk. I'm eager start dogfooding!

🎉 🚀

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.

2 participants