(fix) Wait for slugify to show up before returning slug

[brent-hoover]

Resolves #4011
Impact: major
Type: bugfix

Issue

We weren't waiting for slugify to be resolved and if it wasn't resolved we just returned an empty string, which completely crashes the app. For some reason when reimporting the Basic Product this created a race condition where slugify was not loaded before this product was imported.

Test Problem:

The tests were failing because the code here: was trying to run a Meteor method that was not available yet: (this method literally just returns Meteor.userId())

reaction/server/api/core/core.js

Line 378 in fe7d125

const activeUserId = Meteor.call("reaction/getUserId");

It wasn't an actual test failing but the app not starting. It was the fact that we were masking the failure to get slugify that was allowing tests to pass in the past.

I didn't find a solution to this problem but I did realize that the lazyLoadSlugify function was using getShopId to determine the language but in the context of the server we don't have a user and we should be using the primary shop to determine language, so I created a getPrimaryShopLang function.

Solution

Promise.await rather than resolve. I also removed the check for slugify that just bailed and returned an empty string because I can't see why we would want that.

To fix the getShopId problem we are grabbing the primary shop id when running on the server and the shop of the client when running client code.

Testing

Fix Testing

1. Fully load the app with built in data and devtools loaded. (or you can drop the Products collection after the app is loaded)
2. Choose "Reset Data"
3. Stop the app
4. Restart it
5. Observe that the data loads properly and the app doesn't crash

Regression Testing

1. Create a product and ensure that the slug is created properly
2. Change language to non-latin and run "Fix tests" above

[jshimko]

I think this should do the trick for now. However, note that Promise.await is not in the ES6 spec. It's a method added to Promise by Meteor and wouldn't function outside of the Meteor context. Normally I'd recommend just using async/await with getSlug, but that would cause getSlug to return a Promise instead of a string and you'd have to go update every instance of that function across the app. Probably not worth all of that trouble right now. We can always do that whenever getSlug ends up in a non-Meteor context.

Standby. Failing test may be related to this change.

[jshimko]

This definitely appears to be causing failing tests on both CircleCI and local.

I'm also noticing that getSlug is duplicated in 3 places (client, lib, and server). Do we know if that's necessary? Seems like it shouldn't be since the code is essentially identical in all 3 places.

client
https://github.com/reactioncommerce/reaction/blob/a3b5cb5b7581a1659cb446b564f7bd5341ab5cae/client/modules/core/helpers/utils.js

lib

reaction/lib/api/helpers.js

Lines 14 to 34 in a3b5cb5

	let slugify;
	async function lazyLoadSlugify() {
	let mod;
	// getting the shops base language
	const lang = getShopLang();
	// if slugify has been loaded but the language has changed
	// to be a non latin based language then load transliteration
	if (slugify && slugify.name === "replace" && latinLangs.indexOf(lang) === -1) {
	mod = await import("transliteration");
	} else if (slugify) {
	// if slugify/transliteration is loaded and no lang change
	return;
	} else if (latinLangs.indexOf(lang) >= 0) {
	// if the shops language use latin based chars load slugify else load transliterations's slugify
	mod = await import("slugify");
	} else {
	mod = await import("transliteration");
	}
	// slugify is exported to modules.default while transliteration is exported to modules.slugify
	slugify = mod.default || mod.slugify;
	}

reaction/lib/api/helpers.js

Lines 191 to 209 in a3b5cb5

	/**
	* @name getSlug
	* @method
	* @memberof Helpers
	* @summary return a slugified string using "slugify" from transliteration
	* https://www.npmjs.com/package/transliteration
	* @param {String} slugString - string to slugify
	* @return {String} slugified string
	*/
	export function getSlug(slugString) {
	let slug;
	Promise.await(lazyLoadSlugify());
	if (slugString) {
	slug = slugify(slugString.toLowerCase());
	} else {
	slug = "";
	}
	return slug;
	}

server
https://github.com/reactioncommerce/reaction/blob/a3b5cb5b7581a1659cb446b564f7bd5341ab5cae/server/api/core/utils.js

[brent-hoover]

I don't know about the other places, but the place it's getting called here is from a schema so it seems like it needs to be in lib for that purpose since this code might get called from either client or server. Additionally, maybe I don't understand something but is there a purpose for lazy-loading a library on the server-side. Could we be wrapping these in isServer blocks and just load it at start time?

[brent-hoover]

Tests fail if you remove the code that just swallows the error so...

[brent-hoover]

@jshimko Take a look at this and tell me if you think this makes sense

[jshimko]

Looks like that fixed the failing tests and everything appears to be working as it should.