search

Smithsonian / dpo-voyager

DPO Voyager - 3D Explorer and Tool Suite
Apache License 2.0
169 stars 30 forks source link

make evaluateProps reentrant in all applications #321

Closed sdumetz closed 3 days ago

sdumetz commented 1 month ago

ExplorerApplication.evaluateProps() won't yield the same result when run twice, because const props = this.props only references the original object, so down the line when we get the queryString values, it changes the values stored in this.props, thus potentially changing the result of evaluateProps() in unexpected ways.

I think we should keep this.props unchanged or make it clear we intend to modify it by removing the const props = this.props assignment.

I prefer the first solution as it's a one-line change and allows calling evaluateProps after a change in the page's queryString.

gjcope commented 3 weeks ago

Do you have an example that triggers an issue with the current implementation? I think your change sounds fine, just trying to understand the issue a little better. Thanks!

sdumetz commented 3 weeks ago

It's triggered when you try to control the scene from an external script doing something like this : 

let u = new URL(window.location.href);
u.searchParams.set("root","/scenes/foo");
voyager.reloadDocument();

Clearly not something voyager was made for, but it seems to work well enough with this small change, which is way better than adding a new public method just for this edge case.

gjcope commented 3 weeks ago

Got it, thanks. Sounds good. Merged to https://github.com/Smithsonian/dpo-voyager/tree/rc-46