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

Inconsistent specification of sim-specific features in package.json #1339

Closed
pixelzoom opened this issue Oct 3, 2022 · 11 comments
Closed

Inconsistent specification of sim-specific features in package.json #1339

pixelzoom opened this issue Oct 3, 2022 · 11 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 3, 2022

I'm seeing this pattern in package.json files:

    "simFeatures": {
      "supportsSound": true,
      "supportsInteractiveDescription": true
    },
    "supportsOutputJS": true,
    "supportsDynamicLocale": true

I'm not sure about "supportsOutputJS", but the other 3 properties are definitely sim-specific features. Why are some under "simFeatures", while others (like "supportsDynamicLocale") are not?

I stumbled across this problem while trying to add "supportsDynamicLocale": true to a package.json. I added it to "simFeatures", it didn't work, and I spent time figuring out why.

@jonathanolson you recently added "supportsDynamicLocale". Should it be under "simFeatures"?

@pixelzoom
Copy link
Contributor Author

"colorProfiles" also seems sim-specific, and is not under "simFeatures".

Adding to Dev Meeting project board.

@pixelzoom
Copy link
Contributor Author

It looks like @zepumph renamed “features” to “simFeatures”, but I can’t tell who originally added it. I’m guessing it was someone on the a11y team, since it seems to be mostly (all?) a11y features. There are other properties, like “colorProfiles”, that are clearly sim-specific features and not under “simFeatures”. Adding to the Dev Meeting project board.

@pixelzoom
Copy link
Contributor Author

I'm not 100% certain, but it looks like @zepumph added "simFeatures" (originally named "features") to package.json for #1031.

@zepumph what was the purpose of adding the "simFeatures" property? Should other properties be relocated to be consistent? Or should features under "simFeatures" be moved up a level, and "simFeatures" deleted?

@zepumph
Copy link
Member

zepumph commented Oct 3, 2022

My opinion would be to have the runtime-specific features of the simulation itself be housed in simFeatures. There are an ever growing number, and it has simplified the package files in my opinion. @jonathanolson can we move supportsDynamicLocale into simFeatures?

supportsOutputJS has nothing to do with the features of the runtime, but instead about the source code. It is not a good candidate for nesting in simFeatures.

@zepumph zepumph assigned jonathanolson and unassigned zepumph Oct 3, 2022
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 3, 2022

What other properties should be moved into simFeatures? colorProfiles? Anything else?

@zepumph
Copy link
Member

zepumph commented Oct 5, 2022

Yes, let's move colorProfiles as well. I glanced through many package.json and didn't see any others that seemed like they should be moved.

@jonathanolson
Copy link
Contributor

can we move supportsDynamicLocale into simFeatures?

Yes definitely! I was unaware of simFeatures. colorProfiles seems like it works also.

zepumph added a commit to phetsims/geometric-optics-basics that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/equality-explorer that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/atomic-interactions that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/molecule-shapes-basics that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/equality-explorer-two-variables that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/states-of-matter that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/masses-and-springs-basics that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/acid-base-solutions that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/geometric-optics that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/states-of-matter-basics that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/molecule-shapes that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/rutherford-scattering that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/equality-explorer-basics that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/gravity-and-orbits that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/calculus-grapher that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/my-solar-system that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/blackbody-spectrum that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/ph-scale-basics that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/models-of-the-hydrogen-atom that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/build-a-nucleus that referenced this issue Oct 13, 2022
zepumph added a commit to phetsims/masses-and-springs that referenced this issue Oct 13, 2022
@samreid
Copy link
Member

samreid commented Oct 14, 2022

This commit had at least one problem, seen in phetsims/density@07a3118?diff=split

The JSON came out like:

      "phet": {
        "requirejsNamespace": "DENSITY",
        "phetLibs": [
          "density-buoyancy-common",
          "mobius"
        ],
          "../sherpa/lib/poly-decomp-0.3.0.js",
        "preload": [
          "../sherpa/lib/p2-0.7.1.js",
          "../sherpa/lib/three-r104.js"
        ],

and the unbuilt sim fails with:

Uncaught SyntaxError: Unexpected string (at density_en.html?brand=phet&ea&debugger:40:11)

I'm not sure if other sims are affected.

samreid added a commit to phetsims/density that referenced this issue Oct 14, 2022
@samreid
Copy link
Member

samreid commented Oct 14, 2022

I ran grunt update for density, and it seemed to help.

@zepumph
Copy link
Member

zepumph commented Mar 9, 2023

voicing, colorProfile and dynamicLocale have been moved into simFeatures. I think we are ready to close. @pixelzoom do things look good to you?

@pixelzoom
Copy link
Contributor Author

Confirmed by inspecting geometric-options/package.json. Closing.

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