Skip to content
This repository has been archived by the owner on Feb 4, 2022. It is now read-only.

require saslprep only if it is needed #355

Closed
wants to merge 2 commits into from

Conversation

mcollina
Copy link

@mcollina mcollina commented Sep 10, 2018

Requiring saslprep adds 1s to start an application using MongoDB.
It is especially painful in tests, as certain frameworks spins out
multiple processes (ava, tap). This PR delays the load of saslprep until the last possible minute.

Before: 85553 clinic-flame

After: 88540 clinic-flame

(Taken with node-clinic)

Requiring saslprep adds 1s to start an application using MongoDB.
This is especially painful in tests, as certain frameworks spins out
multiple processes (ava, tap).
@mcollina
Copy link
Author

See reklatsmasters/saslprep#3.

@daprahamian
Copy link
Contributor

@mcollina We have to weigh the trade-offs of deferring the load of saslprep here. In your change, you would add nearly a second to the first authentication request to use saslprep. I've created https://jira.mongodb.org/browse/NODE-1663 to discuss this issue further.

_saslprep = require('saslprep');
} catch (e) {
// don't do anything;
console.warn('Warning: no saslprep library specified. Passwords will not be sanitized');
Copy link
Contributor

Choose a reason for hiding this comment

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

If loading _saslprep fails, can we set _saslprep to an identity function (password => password) ? That way, we only wind up console.warning once.

Copy link
Author

Choose a reason for hiding this comment

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

done.

@daprahamian
Copy link
Contributor

@mcollina Since reklatsmasters/saslprep#3 is resolved, do you still want to merge this in?

@mcollina
Copy link
Author

We can close this.

@mcollina mcollina closed this Sep 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants