TypeError: Cannot read property 'getRandomValues' of undefined

[williamweckl]

Reproduction

model() {
    return this.store.createRecord('profile');
}

Description

Creating a new record to the store raises the error TypeError: Cannot read property 'getRandomValues' of undefined when page is loaded with fastboot.

Probably this commit has introduced the issue: 576a02a#diff-ddf3f6ae6821e1502f6a41d26c52c8a8R13

Versions

Ember cli: 3.13.1
Ember data: 3.13.1
Ember source: 3.12.0

[rwjblue]

Thank you for reporting! Definitely seems like a bug...

[rwjblue]

Doesn't seem like that specific change (576a02a#diff-ddf3f6ae6821e1502f6a41d26c52c8a8R13) did anything but tweak formatting of that line. Seems like the real culprit was the original identifiers PR (#6247).

In the current code, we have this:

data/packages/store/addon/-private/identifiers/utils/uuid-v4.ts

Lines 12 to 15 in 835fd28

	const CRYPTO =
	typeof window !== 'undefined' && window.msCrypto && typeof window.msCrypto.getRandomValues === 'function'
	? window.msCrypto
	: window.crypto;

In FastBoot window is defined, window.crypto is undefined, and window.msCrypto is undefined.

We then call CRYPTO.getRandomValues unconditionally in rng:

data/packages/store/addon/-private/identifiers/utils/uuid-v4.ts

Lines 18 to 23 in 835fd28

	function rng() {
	// WHATWG crypto RNG - http://wiki.whatwg.org/wiki/Crypto
	let rnds8 = new Uint8Array(16);
	return CRYPTO.getRandomValues(rnds8);
	}

Which is called unconditionally in the default export of uuid-v4.ts:

data/packages/store/addon/-private/identifiers/utils/uuid-v4.ts

Lines 61 to 69 in 835fd28

	export default function uuidv4(): string {
	let rnds = rng();
	// Per 4.4, set bits for version and `clock_seq_hi_and_reserved`
	rnds[6] = (rnds[6] & 0x0f) | 0x40;
	rnds[8] = (rnds[8] & 0x3f) | 0x80;
	return bytesToUuid(rnds);
	}

Which is ultimately called from the default identifier generator (which is used during store.createRecord if the user hasn't customized it):

data/packages/store/addon/-private/identifiers/cache.ts

Lines 61 to 70 in 576a02a

	function defaultGenerationMethod(data: ResourceIdentifierObject, bucket: string): string {
	if (isNonEmptyString(data.lid)) {
	return data.lid;
	}
	let { type, id } = data;
	if (isNonEmptyString(id)) {
	return `@ember-data:lid-${normalizeModelName(type)}-${id}`;
	}
	return uuidv4();
	}

@runspired - Mind giving a quick brain dump on how this is supposed to be working in FastBoot? I don't see how CRYPTO (in FastBoot) can ever be something other than undefined, and therefore how store.createRecord could work there...

[ballPointPenguin]

I wonder if this is something that should be improved in fastboot itself, ie incorporating nodejs 'crypto' module and making it available, or if it is more the responsibility of ember-data to provide that.

[rwjblue]

It is definitely possible for ember-data to require('crypto') if that is what we want to do (we'd have to use FastBoot.require though). I'm just not sure what the original design was for this, so wanted to check in with @runspired first...

[runspired]

We would want to require node crypto JIT. Before identifiers, Users shouldn’t have been using createRecord in fastboot though, and it still feels weird to me, although I don’t see much of an alternative pattern in today’s shoebox pattern

[rwjblue]

Doesn't really seem like a shoebox thing to me, calling store.createRecord seems super common for "new" routes why wouldn't you expect that to happen in FastBoot scenarios?

[runspired]

Because there's no way to stash the state for rehydration today. Identifiers at least makes it possible to rehydrate that state, still feels weird to be rehydrating "mutations" in this way.

[rwjblue]

Because there's no way to stash the state for rehydration today.

It is also not about storing partial mutations across the SSR -> client boundary. The pattern I'm talking about is where you'd have a posts.new route that does return this.store.createRecord('post'), and its template would use {{input value=model.title}}. This pattern seems perfectly fine to me, and shouldn't be considered "bad".

[rwjblue]

FWIW, I discussed this more with @runspired in chat. We were talking about different kinds of mutations and completely talking past each other above. Sorry about that @runspired.

RE: this specific issue, we are definitely agree that we need to fix (likely by way of FastBoot.require('crypto').randomBytes in fastboot scenarios). The quick fix will add that code in runtime, but a more thorough fix should attempt to only ship that code in fastboot scenarios.

[rwjblue]

I took a first stab at this over in #6568. I'm not super tied to the specific implementation and if we don't want to go that route we can throw away the implementation commit and just use the test commit.

[williamweckl]

Thank you guys for the amazing job done!