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

jsPsych as global variable #528

Merged
merged 13 commits into from
Aug 15, 2024
Merged

jsPsych as global variable #528

merged 13 commits into from
Aug 15, 2024

Conversation

YUUU23
Copy link
Contributor

@YUUU23 YUUU23 commented Aug 9, 2024

  • when jsPsych instance gets initialized, set global variable window.jsPsych to this object
  • remove all function argument that takes in jsPsych object
  • refactor all usage of jsPsych into window.jsPsych to be accessed globally
  • add util function to get and return the global jsPsych instance
  • tested code under different setting (clinic, home, browser, with video)

Copy link

github-actions bot commented Aug 9, 2024

Visit the preview URL for this PR (updated for commit 99556aa):

https://ccv-honeycomb--pr528-jspsych-global-0rome70j.web.app

(expires Wed, 21 Aug 2024 22:05:08 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 4ace1dcea913a952d2a1af84b94a4421bf36e610

@YUUU23 YUUU23 self-assigned this Aug 9, 2024
@YUUU23 YUUU23 added the 4.0 Versioning: Issue in regards to version 4.0.0 release label Aug 9, 2024
@YUUU23 YUUU23 changed the title Js psych global jsPsych as global variable Aug 9, 2024
@YUUU23 YUUU23 requested a review from eldu August 12, 2024 14:48
Copy link
Contributor

@eldu eldu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you've created a getJsPsych utility function, but are using window.jsPsych in all of the refactored lines.

What reasoning is behind this? From my perspective it seems like you should use the utility function throughout.

@YUUU23
Copy link
Contributor Author

YUUU23 commented Aug 13, 2024

I see that you've created a getJsPsych utility function, but are using window.jsPsych in all of the refactored lines.

What reasoning is behind this? From my perspective it seems like you should use the utility function throughout.

I think when I talked with Rob about this, he noted that maybe first we use the window.jsPsych method for a draft PR to ensure that everything works, and include the utilities function so that it would be more straightforward for developers with not much programming experiences to be able to access jsPsych through a utility function instead of accessing the global variable which is not as straightforward.

I do agree maybe accessing through the utility function throughout Honeycomb may be more consistent and flexible to modifications. The only thing I am unsure about is if it's necessary since accessing this object is only a oneliner.

Please let me know if you have any further thoughts on this! I'm leaning towards refactoring to access through util function throughout.

@YUUU23 YUUU23 requested a review from eldu August 13, 2024 05:49
@eldu
Copy link
Contributor

eldu commented Aug 13, 2024

I see that you've created a getJsPsych utility function, but are using window.jsPsych in all of the refactored lines.
What reasoning is behind this? From my perspective it seems like you should use the utility function throughout.

I think when I talked with Rob about this, he noted that maybe first we use the window.jsPsych method for a draft PR to ensure that everything works, and include the utilities function so that it would be more straightforward for developers with not much programming experiences to be able to access jsPsych through a utility function instead of accessing the global variable which is not as straightforward.

I do agree maybe accessing through the utility function throughout Honeycomb may be more consistent and flexible to modifications. The only thing I am unsure about is if it's necessary since accessing this object is only a oneliner.

Please let me know if you have any further thoughts on this! I'm leaning towards refactoring to access through util function throughout.

Let's use the utility function that we'd be directing folks to use. It might unclear to folks if they see window.jsPsych used everywhere instead of the util function. And it would be good to test to make sure the utils function works (i believe in it fully honestly).

So take out the window.jsPsych and let's use the utility function throughout!

Copy link
Contributor

@eldu eldu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment to the other thread. Basically, let's use the utility function throughout instead of window.jsPsych !

Thanks for the great work Megan !

@YUUU23
Copy link
Contributor Author

YUUU23 commented Aug 14, 2024

I see that you've created a getJsPsych utility function, but are using window.jsPsych in all of the refactored lines.
What reasoning is behind this? From my perspective it seems like you should use the utility function throughout.

I think when I talked with Rob about this, he noted that maybe first we use the window.jsPsych method for a draft PR to ensure that everything works, and include the utilities function so that it would be more straightforward for developers with not much programming experiences to be able to access jsPsych through a utility function instead of accessing the global variable which is not as straightforward.
I do agree maybe accessing through the utility function throughout Honeycomb may be more consistent and flexible to modifications. The only thing I am unsure about is if it's necessary since accessing this object is only a oneliner.
Please let me know if you have any further thoughts on this! I'm leaning towards refactoring to access through util function throughout.

Let's use the utility function that we'd be directing folks to use. It might unclear to folks if they see window.jsPsych used everywhere instead of the util function. And it would be good to test to make sure the utils function works (i believe in it fully honestly).

So take out the window.jsPsych and let's use the utility function throughout!

Just refactored this in the new commit!

@YUUU23 YUUU23 requested a review from eldu August 14, 2024 21:24
YUUU23 and others added 2 commits August 14, 2024 16:37
ref: change all func taking in jspsych to const syntax / trials to co…
Copy link
Contributor

@eldu eldu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work Megan !

@YUUU23 YUUU23 merged commit b969b3d into feat-v4 Aug 15, 2024
8 checks passed
@YUUU23 YUUU23 deleted the jsPsych-global branch August 15, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0 Versioning: Issue in regards to version 4.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants