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

only return an error when trying to load a different version of the JSAPI #28

Closed
tomwayson opened this issue Aug 30, 2017 · 5 comments · Fixed by #48
Closed

only return an error when trying to load a different version of the JSAPI #28

tomwayson opened this issue Aug 30, 2017 · 5 comments · Fixed by #48
Assignees

Comments

@tomwayson
Copy link
Member

tomwayson commented Aug 30, 2017

Currently, we throw return an error when the user calls bootstrap more than once, regardless of:

  • whether or not the first script tag is still loading
  • the first script tag is for the same version of the JSAPI as subsequent script tags

Currently this is causing problems in ember-esri-loader tests (Esri/ember-esri-loader#27). I can work around those problems in that library, but I am wondering if it actually makes sense to return an error in all the above conditions.

I'm pretty sure we want to return an error whenever the user is trying to load a different version of the JSAPI.

If it's the same version of the JSAPI, and if the script has already loaded we should just call the callback w/ window.require as we do here:

script.dataset['esriLoader'] = 'loaded';
// we can now use Dojo's require() to load esri and dojo AMD modules
const dojoRequire = window['require'];
if (callback) {
// let the caller know that the API has been successfully loaded
// and as a convenience, return the require function
// in case they want to use it directly
callback(null, dojoRequire);

If the script hasn't already loaded, I think we want to piggy back on the original onload call.

@tomwayson
Copy link
Member Author

tomwayson commented Aug 30, 2017

Also, I'm fairly certain that this line:

callback(new Error('The ArcGIS API for JavaScript is already loaded.'));

Needs to be guarded with an if (callback) check like we do here:

if (callback) {
// let the caller know that the API has been successfully loaded
// and as a convenience, return the require function
// in case they want to use it directly
callback(null, dojoRequire);
}

@tomwayson tomwayson changed the title When to throw an error when bootsrap() is called more than once When to return an error when bootsrap() is called more than once Aug 30, 2017
@tomwayson
Copy link
Member Author

tomwayson commented Aug 30, 2017

Above I suggest checking whether "the user is trying to load a different version of the JSAPI."

That may be tricky, b/c when they are about to load the second script (and when the first script is still loading), all you have have is the URL, which may not be deterministic.

As a first pass, I'd say it would be sufficient to check if the user is trying to load the exact same script (i.e. same URL). Later we can refine by trying to parse the version out of the URL and if that's successful, check whether or not the user is trying to load the same version.

@tomwayson
Copy link
Member Author

tomwayson commented Aug 30, 2017

Also, whenever we return an error b/c a script is already loading/ed, we should also pass that script as the second argument to the callback as a convenience to the caller (I know that would be helpful to me right now as I'm working through how to handle the above errors in ember-esri-loader).

@tomwayson tomwayson changed the title When to return an error when bootsrap() is called more than once When to return an error when bootstrap() is called more than once Aug 30, 2017
@tomwayson
Copy link
Member Author

tomwayson commented Aug 30, 2017

Ignore that last comment. I think that would be akward w/ the current boostrap() API:

esriLoader.bootstrap((err, dojoRequireOrScriptTagDependingOnWhetherOrNotThereWasAnError) => {...}, options);

A better way to achieve similar would be to export getScript().

@tomwayson
Copy link
Member Author

tomwayson commented Aug 30, 2017

I've implemented the above logic in ember-esri-loader to solve the problems I was having there:

https://github.com/Esri/ember-esri-loader/blob/d4b9d2ac209dea2d9db3be98e817ecb21df92e7f/addon/services/esri-loader.js#L42-L66

I'd like to move all that logic into this library as the resolution to this issue.

We'll probably want to move from using script.onload = () => {} to using addEventListenter when we do.

@tomwayson tomwayson changed the title When to return an error when bootstrap() is called more than once bootstrap() should only return an error when trying to load a different version of the JSAPI Sep 2, 2017
@tomwayson tomwayson self-assigned this Nov 9, 2017
@tomwayson tomwayson mentioned this issue Nov 9, 2017
3 tasks
@tomwayson tomwayson changed the title bootstrap() should only return an error when trying to load a different version of the JSAPI only return an error when trying to load a different version of the JSAPI Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant