Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

the load function does not fail the promise correctly #255

Open
shankari opened this issue May 10, 2023 · 1 comment
Open

the load function does not fail the promise correctly #255

shankari opened this issue May 10, 2023 · 1 comment

Comments

@shankari
Copy link

the $.i18n().load method returns a jquery promise.

function jsonMessageLoader( url ) {

HOWEVER, if the load fails, the promise does not fail. The code explicitly traps the fail callback and converts it to a success callback. It appears that this is to handle 404 exceptions properly via fallbacks. But this also then does not catch other exceptions, such as invalid JSON.

It seems like it would be better to resolve only on the 404 exception, and reject on other exceptions.
I am happy to submit a PR if that would be welcomed.

	function jsonMessageLoader( url ) {
		var deferred = $.Deferred();

		$.getJSON( url )
			.done( deferred.resolve )
			.fail( function ( jqxhr, settings, exception ) {
				$.i18n.log( 'Error in loading messages from ' + url + ' Exception: ' + exception );
				// Ignore 404 exception, because we are handling fallabacks explicitly
				deferred.resolve();
			} );

		return deferred.promise();
	}
shankari added a commit to e-mission/nrel-openpath-join-page that referenced this issue May 11, 2023
- Create an invalid study div with some internal structure
    - error, error details, and contact for assistance
    - invalid div is hidden by default
- Create a function that will hide the valid div and populate and show the invalid div
- Call the function if there are issues while loading the dynamic config
    - use hardcoded values since the i18n might or might not be loaded correctly

I originally assumed that the error would be generated even if the `i18n` load
was not successful, but that is not the case
wikimedia/jquery.i18n#255

I then tried to force such an error

```
            // i18nLoadPromise always resolves with undefined
            // it explicitly catches failure responses and resolves instead
            // https://github.com/wikimedia/jquery.i18n/blob/6afc05ae64a2051d0a97b143ae90ca76f651874e/src/jquery.i18n.messagestore.js#L31
            // I have filed an issue for this
            // wikimedia/jquery.i18n#255
            // while waiting for it to be discussed, we need to look for a workaround
            // only current way to determine whether or not the load succeeded
            // is to check whether there are messages for the currently loaded language
            if (!$.i18n().messageStore.messages['en']) {
                throw SyntaxError("'en' i18n not found or unparseable");
            }
```

but then realized that the join URL was only dependent on the study config and
not the i18n, so even if the i18n was unsuccessful, we would be able to generate the join URL.
So I removed it (although I have recorded it here for posterity). I also did
not include a catch block in the second round of loading.
ANKITSINGH065 added a commit to ANKITSINGH065/jquery.i18n that referenced this issue Jun 8, 2023
@ANKITSINGH065
Copy link

please review @shankari

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants