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

Fix: form fails to load when no media manifest is provided #332

Merged

Conversation

eyelidlessness
Copy link
Contributor

This ensures manifest is always present on a Survey before preparing the media mapping for transform. An alternative would be to handle the optional case, but it felt like it makes more sense not to intentionally introduce more optional properties.

For some reason these resolutions weren't automatically populated on previous NPM install?
In the future, nested configs should extend the baseline, but trying to keep a light touch for this fix.
@eyelidlessness
Copy link
Contributor Author

This change also:

  • Updates package-lock.json to include resolution/integrity data for packages updated in Fix: resolve jr: URLs with spaces #291. It's unclear why NPM did not do that previously.

  • Updates ESLint configs to specify a consistent ecmaVersion (to permit object spread as in the communicator.js change). ES2020 and below is supported in our targeted Node versions, with one false negative and two easily identifiable exceptions:

    • The RegExp.prototype.flags test fails since Node 16, but only because it supports an additional flag that's not tested
    • Atomics.wake is undefined
    • In Node 14, attempting to spread arguments in a method call after optional chaining (foo?.bar(...[])) will produce an error because it was previously implemented as sugar for foo.bar.apply(null, [])

    I'd be okay reverting the ESLint change for now if preferred, but it should happen at some point if so.

@lognaturel
Copy link
Contributor

lognaturel commented Oct 14, 2021

That means that Transformer got a breaking change, right? Is that really a desirable thing? If so, shouldn't it be released as a major version? Was https://github.com/enketo/enketo-express/pull/291/files#diff-0e6c59a8df2414ee19f0d9beabc1ca0da139de21a736c6125a8247d25b85da38R109 what caused the issue?

@eyelidlessness
Copy link
Contributor Author

We discussed in Slack, but adding here for posterity, this was the error stack:

TypeError: Cannot read property 'map' of undefined
        at toMediaMap (/Users/gnosis/Projects/ODK/enketo-express/app/lib/url.js:29:14)
        at /Users/gnosis/Projects/ODK/enketo-express/app/controllers/transformation-controller.js:112:24
        at processTicksAndRejections (node:internal/process/task_queues:96:5)

It would be easy to see that and think "this is in transformer", but in this case it was just a call to express's toMediaMap which expects a manifest array, which is not present for a form with no media and thus no manifest.

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.

Confirmed that I understand the issue and fix. No issues with the config changes.

@eyelidlessness eyelidlessness merged commit 755f754 into enketo:qa/3.0.2-pre Oct 14, 2021
@eyelidlessness eyelidlessness mentioned this pull request Oct 20, 2021
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.

None yet

2 participants