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

pattern for stripping binder documentation #23

Open
pixelzoom opened this issue Oct 15, 2018 · 6 comments
Open

pattern for stripping binder documentation #23

pixelzoom opened this issue Oct 15, 2018 · 6 comments

Comments

@pixelzoom
Copy link

pixelzoom commented Oct 15, 2018

I see 12 occurrence of

assert && phet.chipper.queryParameters.binder ...

For example, in Slider.js:

// support for binder documentation, stripped out in builds and only runs when ?binder is specified
assert && phet.chipper.queryParameters.binder && InstanceRegistry.registerDataURL( 'sun', 'Slider', this );

Using assert in this way (to strip out things other than assert) seems a bit questionable. Labeling for discussion at developer meeting.

@zepumph
Copy link
Member

zepumph commented Oct 25, 2018

Is the goal is hiding behind assert to make sure it only runs in development mode?

@pixelzoom
Copy link
Author

Based on comment "...stripped out in builds and only runs when ?binder is specified", that appears to be the goal.

@jonathanolson
Copy link
Contributor

If it helps, it's possible to override other globals as false during the uglify step, so that simplification will strip things out of builds.

@pixelzoom
Copy link
Author

pixelzoom commented Nov 29, 2018

11/29/18 dev meeting:

@samreid options: (1) get rid of assert && and get a few more chars in the html file, or (2) create a new flag like 'isRequirejsMode`.

@zepumph create a lint rule to allow only assert && assert, not assert && somethingElse. This would prevent putting assert && before a block of assertions, or assert && Object.freeze.

@mbarlow12 Not sure what direction binder is going to go in. This approach gets SVGs from the sim. So wondering if this is even something that we want to keep.

@jonathanolson @samreid assert && Object.freeze is probably OK, since we'd never want to do freeze without assertions enabled.

Assigned to @mbarlow12 to determine whether this is needed for binder. If not, remove it. If it is needed, then bring it up for discussion again at dev meeting.

@samreid If it's needed, then using a different flag is a likely option.

@zepumph
Copy link
Member

zepumph commented Sep 30, 2019

Note that there are now 57 usages of this pattern.

@zepumph
Copy link
Member

zepumph commented Jul 7, 2020

I think that the way forward would be a new global that is set to false when building code, so that the optimizer strips this out. I think that a general thing, like "isUnbuilt" seems better that something specific like "usingBinder." This way we only need to touch the build process to support this once. I will add this to my plate at a low priority.

@zepumph zepumph self-assigned this Jul 7, 2020
@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
Projects
None yet
Development

No branches or pull requests

4 participants