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

Update documentation throughout the code that references RequireJS #905

Closed
jessegreenberg opened this issue Feb 27, 2020 · 14 comments
Closed

Comments

@jessegreenberg
Copy link
Contributor

Documentation about RequireJS no longer applies since we have switched to ES6 modules. For instance from phet-software-design-patterns.md

Since requireJS does not do well with circular dependencies, two types can refer to each other through the namespace.

And this comment in GrabDragInteraction

// wrap in a function because phet.joist.sim doesn't exist at RequireJS time

We should do an inspection to either update comments to reflect "compilation free mode" or remove entirely if they no longer apply.

@jonathanolson @samreid how should we do this, is this a potential chip away issue to be divvied up like phetsims/tasks#1017?

@jessegreenberg
Copy link
Contributor Author

kite, mobius, scenery namespaces have comments like

The main 'kite' namespace object for the exported (non-Require.js) API. Used internally since it prevents Require.js issues with circular dependencies.

@samreid
Copy link
Member

samreid commented Mar 1, 2020

I'm also seeing that chipper/package.json still refers to requirejs.

@jonathanolson
Copy link
Contributor

Sounds like we'll want to do a project-wide search and try to replace things with improved terminology.

@jonathanolson jonathanolson removed their assignment Mar 5, 2020
@samreid
Copy link
Member

samreid commented Mar 6, 2020

That sounds like something a primary developer could take the lead on, and request help if they come across parts where it is unclear how to proceed. @ariel-phet would you like to recommend someone to take the lead on this? It could be any developer.

@ariel-phet
Copy link
Contributor

@jessegreenberg is willing to take this on, and will call on other devs as needed

@zepumph
Copy link
Member

zepumph commented Apr 2, 2020

I also think that there is a bigger discussion to be had here about the vestigial, but widely used naming of REQUIREJS_NAMESPACE formatting. You can search for usages of requirejsNamespace including that field in every package.json.

@jessegreenberg
Copy link
Contributor Author

Right, and it is still used throughout chipper @jonathanolson do you have a recommendation for a rename of this field?

jessegreenberg added a commit to phetsims/aqua that referenced this issue Apr 6, 2020
jessegreenberg added a commit to phetsims/aqua that referenced this issue Apr 6, 2020
@jonathanolson
Copy link
Contributor

do you have a recommendation for a rename of this field?

It accurately describes its legacy purpose, but I'm also not against other people wanting to rename it. Not attached either way.

@jonathanolson jonathanolson removed their assignment Apr 6, 2020
@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Apr 6, 2020

I realized I am not knowledgeable enough about requireJS and how various things have changed with ES6 modules to do this. I have assembled a list of about 2 dozen questions that are of the form "should the documentation change or does the implementation need to change", and I am still not done going through everything. So I would recommend that a responsible developer for each repository go through first and review references. Here is the main list of places I am seeing them.

Adding to developer meeting to check in, make sure responsible devs are correct, and assign to the few repos that are missing names.

jonathanolson added a commit to phetsims/utterance-queue that referenced this issue Jun 17, 2020
jonathanolson added a commit to phetsims/vector-addition that referenced this issue Jun 17, 2020
jonathanolson added a commit to phetsims/vector-addition-equations that referenced this issue Jun 17, 2020
jonathanolson added a commit to phetsims/wave-interference that referenced this issue Jun 17, 2020
jonathanolson added a commit to phetsims/wave-on-a-string that referenced this issue Jun 17, 2020
jonathanolson added a commit to phetsims/gravity-force-lab-basics that referenced this issue Jun 17, 2020
jonathanolson added a commit to phetsims/phet-info that referenced this issue Jun 18, 2020
@jonathanolson
Copy link
Contributor

Should be complete, a lot of work done in 0e01ac5 removes require.js compatibility in a lot of our build files, and gets rid of a lot of vestigial stuff (e.g. global.phet.chipper). @ariel-phet A brief review by another dev would probably be good.

@chrisklus
Copy link
Contributor

from 06/18/20 dev meeting: @samreid volunteered to review. thanks!

@samreid
Copy link
Member

samreid commented Jun 19, 2020

In Slack, @jonathanolson said:

Main review thing is basically that require/define were removed from our browser lint globals + I stripped out ways where files could be used from require.js or node.js (we only needed them in node.js now) + chipperGlobals went away

To review, I took the following steps:

  • Examined the change set
  • Looked for other occurrences of typeof define !== 'undefined' and found none
  • Looked for other occurrences of global.define and found none
  • Tested natural selection in unbuilt mode briefly
  • Tested phet and phet-brand builds of natural selection briefly

No problems noted, nice work @jonathanolson, 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

8 participants