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

Feature/swagger #1480

Merged
merged 12 commits into from
Sep 9, 2016
Merged

Feature/swagger #1480

merged 12 commits into from
Sep 9, 2016

Conversation

marla-singer
Copy link
Contributor

@marla-singer marla-singer commented Sep 2, 2016

Closes #1476

@brylie
Copy link
Contributor

brylie commented Sep 5, 2016

Remember to add an ignore rule to our .eslintignore so that Swagger UI code is not checked.

@brylie
Copy link
Contributor

brylie commented Sep 5, 2016

Also, try importing the Swagger UI CSS directly from the NPM package:

@marla-singer
Copy link
Contributor Author

@brylie thanks for link!
Whether it is necessary to display buttons "Authorize" and "Explore" or these buttons don't need in apinf project?

@bajiat
Copy link
Contributor

bajiat commented Sep 5, 2016

@marla-singer The buttons are needed. Although, I think the presence of the Authorize button depends on the Swagger file - it is only available if the authorization has been defined in the Swagger file to be displayed.

@marla-singer
Copy link
Contributor Author

@brylie @bajiat I come in to collision:

  1. I use package "swagger-ui-browserify" that base on 2.1.4 version of "swagger-ui". It works, but Authorization button is not avaible because this feature was appeared in ^2.2 Swagger version.
  2. So I try to use package "swagger-ui" in new empty meteor project and have error on console
Uncaught TypeError:template is not a function

which i can't fix it quickly and can't find solution on the Enthernet.

@marla-singer
Copy link
Contributor Author

@brylie @bajiat Well, I did fork of swagger-ui-browserify (SUB) repo, updated swagger-ui to latest version and fixed errors. After I created new meteor project with my SUB package version and run it. You can look sources here. Also I added some instruction to run project.

Can I integrate this private SUB package into apinf project? Package is available here and PR was created too

@brylie brylie self-assigned this Sep 7, 2016
@brylie
Copy link
Contributor

brylie commented Sep 7, 2016

@marla-singer good work.

We basically need to be able to duplicate what you did in the 'empty-project', but within the Apinf project. E.g. we want to be able to call the following code from within Apinf:

import { SwaggerUi } from 'swagger-ui-browserify'`

const swagger = new SwaggerUi({
    url: '/swagger.json',
    dom_id: 'swagger-ui-container',
    useJQuery: true,
    supportHeaderParams: true,
    onComplete: function (swaggerApi, swaggerUi) {
      console.log('Loaded SwaggerUI')
    },
    onFailure: function (error) {
      console.error('Unable to Load SwaggerUI', error)
    },
    docExpansion: 'none'
  });

swagger.load()

If possible, we would like to install Swagger UI in our project using npm:

npm install --save swagger-ui-browserify

@marla-singer
Copy link
Contributor Author

@brylie Yes, I see that attach swaggerUi to window is a bad idea but Swagger developers use swaggerUi variable in library like global. It won't work if I create variable without window.swaggerUi. I need more time to explore this problem and find some solution.

@marla-singer
Copy link
Contributor Author

@brylie Official npm package "swagger-ui-browserify" bases on swagger-ui version 2.1.4 and doesn't provide an implementation of Authorization button

@brylie
Copy link
Contributor

brylie commented Sep 7, 2016

@marla-singer your pull request was merged into swagger-ui-browserify, updating the swagger-ui dependency to 2.2.3.

@brylie
Copy link
Contributor

brylie commented Sep 7, 2016

I see that attach swaggerUi to window is a bad idea but Swagger developers use swaggerUi variable in library like global. It won't work if I create variable without window.swaggerUi.

Do you have an upstream support request regarding this issue?

@brylie
Copy link
Contributor

brylie commented Sep 7, 2016

@bajiat should we go ahead and use the swagger-ui-browserify package, with the expectation that @marla-singer's changes will be published soon?

This will mean that the Authorization button may not be in @marla-singer's PR, but should appear when the upstream package is updated.

@bajiat
Copy link
Contributor

bajiat commented Sep 7, 2016

@brylie @marla-singer It is ok to continue and use the swagger-ui-browserify particularly if they are planning to publish it in the next couple of days. If we need to upgrade the package, we can do that in connection with the Swagger UI continuation task(s) we have in backlog.

@marla-singer
Copy link
Contributor Author

@brylie Please review.
I used swagger-ui-browserify v2.2.3 where don't resolve problem with attach swagger to window. So I suggest to improve it after developer will publish patch.

@brylie
Copy link
Contributor

brylie commented Sep 9, 2016

Cool, for now we can attach it to window. Can we remove the SwaggerUi object from window when the documentation browser is not visible? E.g.

Template.swaggerUi.onDestroyed({
  // Remove Swagger UI from window
  delete window.SwaggerUi;
});

@brylie brylie merged commit 55f6b27 into develop Sep 9, 2016
@brylie brylie deleted the feature/swagger branch September 9, 2016 06:52
@brylie brylie removed the in progress label Sep 9, 2016
@brylie
Copy link
Contributor

brylie commented Sep 9, 2016

I went ahead and merged this PR. We can come back to the window cleanup or find an alternative approach as a follow-up task.

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.

None yet

3 participants