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

Diff wrapper isn't working #281

Closed
liammulh opened this issue May 18, 2020 · 12 comments
Closed

Diff wrapper isn't working #281

liammulh opened this issue May 18, 2020 · 12 comments

Comments

@liammulh
Copy link
Member

For phetsims/qa#501.

The Problem

If you paste https://phet-dev.colorado.edu/html/states-of-matter/1.2.0-dev.4/phet-io/ (the wrapper landing page) in and hit compare, you get:

"New sim: https://phet-dev.colorado.edu/html/states-of-matter/1.2.0-dev.4/phet-io/states-of-matter_all_phet-io.html?postMessageOnError"

but it should say something about finding 0 differences.

Source of Confusion

@KatieWoe and I are confused as to why this line exists in the diff wrapper:

"The URL can be the wrapper landing page (the simulation/version/ directory), the simulation HTML file, or the phet-io.js lib file."

If you paste in https://phet-dev.colorado.edu/html/states-of-matter/1.2.0-dev.4/, and hit compare, you get nothing. Should this be changed to:

"The URL can be the wrapper landing page (the simulation/version/phet-io/ directory), the simulation HTML file, or the phet-io.js lib file."?

@KatieWoe
Copy link
Contributor

Also, are the html file and phet-io.js lib file accurate? This was not something we were instructed to test when this was last discussed, but it looks like it should be.

@chrisklus
Copy link
Contributor

In https://github.com/phetsims/phet-io/issues/1648 we turned off generation of the phet-io API file on build because it was causing problems, and there's no longer an existing API file because we stopped checking those in. So when the sim is built, it currently has no way to get an api file (note the 404 when you click the "PhET-iO API JSON" link at the bottom of the wrapper index).

So I don't think there is any reason to support or test the diff wrapper for the time being. Sorry for the confusion!

@samreid or @zepumph please comment if you have any other thoughts.

@jbphet
Copy link
Contributor

jbphet commented May 19, 2020

@samreid - you marked the priority for this as 'high' and left it assigned to me, but I'm not clear what I should do about it. From @chrisklus's comment above, it sounds like we should just ignore this problem and close the issue. True?

@jbphet jbphet assigned samreid and unassigned jbphet May 19, 2020
@samreid
Copy link
Member

samreid commented May 19, 2020

In https://github.com/phetsims/phet-io/issues/1648 and https://github.com/phetsims/phet-io/issues/1664 we will need to re-enable API file generation to unblock publication.

@samreid samreid removed their assignment May 19, 2020
@zepumph
Copy link
Member

zepumph commented May 21, 2020

I wanted to also tag #280, as this is basically the same issue (no api files on build right now).

@jbphet
Copy link
Contributor

jbphet commented May 29, 2020

@arouinfar and I reviewed this on 5/29/2020 in an effort to identify issues that need to be resolved prior to making an early customer dev release. When reading through the description, it sounds like this issue would need to be resolved prior to such a release, yet it is marked as unassigned and on-hold. @samreid or @zepumph - can you update this to make it clear whether we can publish to a customer prior to resolving this (and why)?

@jbphet
Copy link
Contributor

jbphet commented May 29, 2020

I've marked this as high priority so that it gets seen and responded to. If it's really a non-issue for publication, feel free to modify that level.

@samreid
Copy link
Member

samreid commented May 30, 2020

This blocks publication for an RC or production, but will not be used for a client dev release.

@zepumph
Copy link
Member

zepumph commented Jun 4, 2020

@KatieWoe can you please confirm this is fixed in phetsims/qa#505

@zepumph zepumph assigned KatieWoe and unassigned samreid, chrisklus and zepumph Jun 4, 2020
@zepumph
Copy link
Member

zepumph commented Jun 4, 2020

"The URL can be the wrapper landing page (the simulation/version/phet-io/ directory), the simulation HTML file, or the phet-io.js lib file."?

@muedli, sorry these instructions are not set up for dev versions. They are for published phet-io versions, in which simulation/version/ would be the root (what phet-io/ is for dev versions). Sorry for the confusion during dev testing.

@KatieWoe
Copy link
Contributor

KatieWoe commented Jun 4, 2020

It does seem to be working in phetsims/qa#505. Tested with wrapper index url

@KatieWoe KatieWoe assigned zepumph and unassigned KatieWoe Jun 4, 2020
@zepumph
Copy link
Member

zepumph commented Jun 5, 2020

Thanks! I'm going to close this issue then.

@zepumph zepumph closed this as completed Jun 5, 2020
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

6 participants