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

utterance-queue is dependent on scenery #33

Open
jessegreenberg opened this issue Aug 25, 2021 · 8 comments
Open

utterance-queue is dependent on scenery #33

jessegreenberg opened this issue Aug 25, 2021 · 8 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

utterance-queue currently has a dependency on scenery because of a usage of PDOMUtils in AriaHerald. But scenery depends on utterance-queue, so this introduces a circular dependency. How should we handle? Move PDOMUtils to something that is a dependency for both scenery and utterance-queue? Move just the used function? Duplicate the used function?

@jessegreenberg
Copy link
Contributor Author

I think PDOMUtils has too many things that are scenery specific or unique to PDOM to move the whole file out of scenery. I recommend we make a new file called DOMUtils.js in phet-core that will have setTextContent and other things that are useful for DOM operations but not unique to PDOM related work.

@zepumph does this seem OK to you? If so, reassign and I am happy to do this.

@zepumph
Copy link
Member

zepumph commented Aug 26, 2021

Yes, I think that is a really good idea. Thanks!

@jessegreenberg
Copy link
Contributor Author

I tried to do this but setTextContent uses validate which lives in axon. I don't think we should introduce axon as a dependency of phet-core because axon already depends on phet-core and that would trade this circular dependency for another.

@jessegreenberg
Copy link
Contributor Author

I wondered about moving validate out of axon because it seems useful for more than just observables and inquired about that in phetsims/axon#361. I think this is on hold until we get a yay or nay over there.

@jessegreenberg
Copy link
Contributor Author

While discussing phetsims/axon#361, @samreid suggested that the validate portion of setTextContent could just be removed so that we could move it into phet-core. We can copy the regex in ValidatorDef.STRING_WITHOUT_TEMPLATE_VARS_VALIDATOR, or move IT into phet-core to be used in scenery.

@jessegreenberg
Copy link
Contributor Author

I tried this and noticed that a number of things would have to move from PDOMUtils to DOMUtils to support this (trimLeft, INPUT_TAG, tagNameSupportsContent, ELEMENTS_WITHOUT_CLOSING_TAG, containsFormattingTags).

Actually the majority of PDOMUtils is not specific to PDOM and could make sense in a DOMUtils. I think going through the following steps to move most of PDOMUtils into phet-core would preserve history best.

  1. Rename PDOMUtils to DOMUtils.
  2. Move DOMUtils to phet-core using copy-history-to-different-repo.sh
  3. Re-create PDOMUtils in scenery and move portions of DOMUtils that are specific to the PDOM PDOMUtils.

Would that work and make sense? Alternatively, both PDOMUtils and DOMUtils will exist at the end of this and a commit message like "move many PDOMUtils functions into phet-core so utterance-queue doesn't have to depend on scenery" will link history between the two.

I am happy to do either but don't want to mess up the history in this move! @zepumph which would you recommend? I ask since you have knowledge of copy-history-to-different-repo and have expressed an interest in preserving history in the past.

@zepumph
Copy link
Member

zepumph commented Feb 16, 2022

I like this idea! I thought that it would be best to move to phet-core, but it depends on axon, so utterance-queue sounds good for now.

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Feb 16, 2022
@zepumph
Copy link
Member

zepumph commented Apr 1, 2022

From meeting with JG today, we will rename to DOMUtils and move to utterance-queue using the copy history script.

@zepumph zepumph self-assigned this Apr 1, 2022
@zepumph zepumph removed their assignment Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants