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

Sim specific PhET-iO wrappers not being copied during build #785

Closed
samreid opened this issue Aug 5, 2019 · 7 comments
Closed

Sim specific PhET-iO wrappers not being copied during build #785

samreid opened this issue Aug 5, 2019 · 7 comments

Comments

@samreid
Copy link
Member

samreid commented Aug 5, 2019

Discovered in https://github.com/phetsims/phet-io-wrapper-sonification/issues/84. It seems the ability to build a sim-specific PhET-iO wrapper was dropped, probably circa 34923a2 (part of #680) or thereabouts.

samreid added a commit to phetsims/hookes-law that referenced this issue Aug 5, 2019
samreid added a commit that referenced this issue Aug 5, 2019
@samreid
Copy link
Member Author

samreid commented Aug 5, 2019

Proposed fixes pushed, it seemed straightforward. @zepumph can you please review?

@samreid samreid removed their assignment Aug 5, 2019
@KatieWoe
Copy link

KatieWoe commented Aug 5, 2019

CT seems to be showing errors. @zepumph mentioned it may be related to this issue.

samreid added a commit that referenced this issue Aug 5, 2019
zepumph added a commit to phetsims/forces-and-motion-basics that referenced this issue Aug 6, 2019
zepumph added a commit to phetsims/capacitor-lab-basics that referenced this issue Aug 6, 2019
zepumph added a commit to phetsims/beers-law-lab that referenced this issue Aug 6, 2019
zepumph added a commit to phetsims/arithmetic that referenced this issue Aug 6, 2019
@zepumph
Copy link
Member

zepumph commented Aug 6, 2019

The changes look good. Thanks @samreid. Review from here:

  • const simSpecificWrappers = packageObject.phet && packageObject.phet[ 'phet-io' ] && packageObject.phet[ 'phet-io' ].wrappers ? packageObject.phet[ 'phet-io' ].wrappers : [];

    Instead you could use _.at(packageObject, "phet['phet-io'].wrappers") which will return [undefined] if it doesn't exist. Just a thought, no strong preference.

  • there are still dedicated wrappers in chipper/data/wrappers. They should be removed and the same process done to them. I made a commit to review.

@zepumph zepumph assigned samreid and unassigned zepumph Aug 6, 2019
@samreid
Copy link
Member Author

samreid commented Aug 7, 2019

I considered

// A
_.at( packageObject, "phet[ 'phet-io' ].wrappers" );// eslint-disable-line 

// B
_.at( packageObject, 'phet[\'phet-io\'].wrappers' );

// C
_.at( packageObject, 'phet["phet-io"].wrappers' );

I prefer the former because searches for phet[ 'phet-io' ] won't be missed. However, I realized that we would need to guard on undefined to prevent processing that. Maybe simpler to improve formatting of the prior strategy?

samreid added a commit that referenced this issue Aug 7, 2019
@samreid
Copy link
Member Author

samreid commented Aug 7, 2019

Also, I reviewed the changes that moved classroom activity, lab book and arithmetic wrappers out. Tested building and running arithmetic's wrapper. Looks great, closing.

@samreid samreid closed this as completed Aug 7, 2019
@zepumph
Copy link
Member

zepumph commented Aug 7, 2019

No need to reopen if you still like your way more, I just wanted to post what the difference would be. I think I prefer the latter, but likely only because _.at is new and fancy to me.

  // Add sim-specific wrappers
  const simSpecificWrappers = packageObject.phet &&
                              packageObject.phet[ 'phet-io' ] &&
                              packageObject.phet[ 'phet-io' ].wrappers ?
                              packageObject.phet[ 'phet-io' ].wrappers : [];

// vs

  const simSpecificWrappers = _.at( packageObject, 'phet[ "phet-io" ].wrappers' )[ 0 ] ?
                              _.at( packageObject, 'phet[ "phet-io" ].wrappers' )[ 0 ] : [];

@samreid
Copy link
Member Author

samreid commented Aug 8, 2019

It's a good use of _.at and much more concise, but my concern is that it impedes text-searchability of the term phet[ 'phet-io' ].

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

No branches or pull requests

3 participants