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

remove inline-code #7905

Merged
merged 10 commits into from
Mar 23, 2022
Merged

Conversation

danielleontiev
Copy link
Contributor

@danielleontiev danielleontiev commented Mar 11, 2022

This is the follow-up for #7295

I've rebased it on master and fix conflicts. Author of the changes in git is still @AndreKoepke, I am only mentioned as commiter.

I've tested it in IDE-driven server, built docker image and run it, and run npm run test - everything looks good.

@tim-lai, please check this one

@danielleontiev danielleontiev mentioned this pull request Mar 11, 2022
17 tasks
@danielleontiev
Copy link
Contributor Author

Are any steps required from me, @tim-lai?

Copy link
Contributor

@tim-lai tim-lai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @danielleontiev, thanks for the followup PR. I am encountering two issues with this PR when used with Docker.

Case 1: provide a different url

docker build -t pr/7905 .

docker run --rm -p 80:8080 -e SWAGGER_JSON_URL="https://petstore3.swagger.io/api/v3/openapi.json" -v `pwd`:/app pr/7905

Case 2: provide a local file

docker build -t pr/7905 .

docker run --rm -p 80:8080 -e SWAGGER_JSON=/foo/sample-multipart-oas3.json -v $(pwd)/dev-helpers/examples:/foo pr/7905

I've confirmed both cases work me locally (Mac) with current master branch. Also fyi, I've included the Docker build step in both cases, but one only needs to build the image once if only switching between the two Docker run commands.

@danielleontiev
Copy link
Contributor Author

Thank you for attentive testing! Could you please describe some more details about what issues do you encounter using docker? Because I've tried on MacOS (using podman) and on Linux with native docker installation and have not faced any issues. My steps:

git clone git@github.com:danielleontiev/swagger-ui.git
cd swagger-ui
docker build -t swagger-ui-master --no-cache .
docker run --rm -d -p 8080:8080 -e SWAGGER_JSON_URL="https://petstore3.swagger.io/api/v3/openapi.json" swagger-ui-master # run master image on 8080
git checkout remove-inline-code
docker build -t swagger-ui-branch --no-cache .
docker run --rm -d -p 8081:8080 -e SWAGGER_JSON_URL="https://petstore3.swagger.io/api/v3/openapi.json" swagger-ui-branch # run branch image on 8081

Master image:

image

Branch image:

image

Besides successful page load all buttons seems to work - I can send requests to petstore and recieve responses

@vladislavsheludchenkov
Copy link

@tim-lai I've checked it additionally with Docker Desktop on macOS, and my results are similar to @danielleontiev. Container starts without any issues, page loads, requests work and no errors are in the browser console. I've used your exact commands both on master and remove-inline-code branches.

@tim-lai
Copy link
Contributor

tim-lai commented Mar 21, 2022

@danielleontiev @OrangeMemes What I am observing is that Docker is always loading the remote OAS3 json file. It's not observing the -e parameter.

So for the remote URL case, these two scripts load the same page and definition:

expect match with OAS2 screenshot:

docker run --rm -p 80:8080 -e SWAGGER_JSON_URL="https://petstore.swagger.io/v2/swagger.json" pr/7905

Petstore-OAS2

expect match with OAS3 screenshot(currently always getting this):

docker run --rm -p 80:8080 -e SWAGGER_JSON_URL="https://petstore3.swagger.io/api/v3/openapi.json" pr/7905

Petstore-OAS3

For local file, it's the same... still loading the remote OAS3 definition. Fyi, my script in earlier comment uses a file located in <project>/dev-helpers/examples/<sample_definition>.json.

Copy link
Contributor

@tim-lai tim-lai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve merge conflicts from a24860c

dev-helpers/dev-helper-initializer.js Show resolved Hide resolved
@danielleontiev
Copy link
Contributor Author

@tim-lai is it possible that you are facing some obscure browser caching behavior? It works for me with both remote url and with local file mounted inside docker container.

@tim-lai tim-lai merged commit ec51dc3 into swagger-api:master Mar 23, 2022
@tim-lai
Copy link
Contributor

tim-lai commented Mar 23, 2022

@danielleontiev

is it possible that you are facing some obscure browser caching behavior? It works for me with both remote url and with local file mounted inside docker container.

Hmm, interestingly, working off a fresh Firefox update instead of Chrome, the two cases I cited are both working. Not sure on the Chrome status. I'll merge this PR for now, and monitor for any aftereffects. Thanks for the continued followup discussion!

@danielleontiev
Copy link
Contributor Author

Thank you!

@danielleontiev danielleontiev deleted the remove-inline-code branch March 24, 2022 08:31
adamw pushed a commit to softwaremill/tapir that referenced this pull request Mar 28, 2022
This update requires changes in SwaggerUI because this PR
(swagger-api/swagger-ui#7905) removes inline
code from index.html where the hack with replacing petstore link was
implemented. Now, petstore link lives inside swagger-initializer.js and
replacing is now performed for the swagger-initializer.js file.
JuanSW18 pushed a commit to Digital-Paw/digital-paw-swagger-ui that referenced this pull request Aug 23, 2024
* apply /dist changes to /dev-helpers

* add missing "useBasicAuthenticationWithAccessCodeGrant: false" after merge conflict

Co-authored-by: akop <akop@ppi.de>
Co-authored-by: Tim Lai <timothy.lai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants