Skip to content

LG-3474: Reference Acuant SDK as static resource from root#4269

Merged
aduth merged 4 commits intomasterfrom
aduth-lg-3474-acuant-sdk-direct
Oct 6, 2020
Merged

LG-3474: Reference Acuant SDK as static resource from root#4269
aduth merged 4 commits intomasterfrom
aduth-lg-3474-acuant-sdk-direct

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Oct 5, 2020

Why: To avoid unnecessary server processing

As a user, I don't want the document capture experience to take a long time to load, so that I can upload my images without being confronted by spinners

Implementation notes:

From what I've been able to tell, Acuant SDK does make an attempt to load additional assets from the loaded script's directory, at least for AcuantImageProcessingService.js.mem. This compiled binary is responsible for loading AcuantImageProcessingWorker, which does load from the current directory. As such, only these files are necessary to load as relative, which could at least avoid server handling AcuantJavascriptWebSdk.min.js (1MB) and AcuantImageProcessingService.js.mem (10KB).

The relevant code is minified in the Acuant SDK (even in the non-minified source file), but there is an expanded copy in the React source example:

Open questions:

  • Is it worth it? The original ticket would have use measuring the exact benefit. It seems like we should want to not have Rails serve these files if possible? Assuming we're using a server like nginx to serve static files (needs clarity).
  • Previously we would set the content type header for .js.mem file. This is no longer implemented. Locally, it falls back to text/plain, which at first glance does not appear to cause any problems.

aduth added 2 commits October 5, 2020 11:27
**Why**: To avoid unnecessary server processing

As a user, I don't want the document capture experience to take a long time to load, so that I can upload my images without being confronted by spinners
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, we do serve the public/ directory via Nginx in prod currently, so this should work

@aduth
Copy link
Contributor Author

aduth commented Oct 5, 2020

One thing I should double-check here is whether this can work if we'd relied on the server to send CSP directives.

These:

SecureHeaders.append_content_security_policy_directives(
request,
script_src: ['\'unsafe-eval\''],
)

@zachmargolis
Copy link
Contributor

One thing I should double-check here is whether this can work if we'd relied on the server to send CSP directives.

We set our nginx headers here: https://github.com/18F/identity-devops/blob/master/kitchen/cookbooks/login_dot_gov/templates/default/nginx_server.conf.erb

I forget if the way CSP works is if we need to set the unsafe-eval on the HTML document that loads the JS, or on the JS file itself?

@jmhooper
Copy link
Contributor

jmhooper commented Oct 5, 2020

I forget if the way CSP works is if we need to set the unsafe-eval on the HTML document that loads the JS, or on the JS file itself?

You need it on the document. We set them on the JS in the SDK controller because Safari has some weird behavior if a CSP header is present on JS files. The assets we are serving from /public should not have a CSP and not encounter those issues.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

@aduth aduth merged commit 191436d into master Oct 6, 2020
@aduth aduth deleted the aduth-lg-3474-acuant-sdk-direct branch October 6, 2020 12:18
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.

3 participants