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

Overwriting /api #71

Open
t4t5 opened this issue Apr 25, 2017 · 9 comments
Open

Overwriting /api #71

t4t5 opened this issue Apr 25, 2017 · 9 comments

Comments

@t4t5
Copy link

t4t5 commented Apr 25, 2017

I have a little problem. I started using jus (awesome project btw) to generate some docs for the upcoming SweetAlert release.

I want to have a /api route that explains the library's API. However, it seems like that URL is occupied in order to show some internal stuff. :)

screen shot 2017-04-25 at 17 44 39

Would you consider changing that URL, or at least making it overwriteable? I think having an /api route is a pretty common use case for library documentation (which seems to be the primary usage of jus)!

@jdormit
Copy link
Member

jdormit commented Apr 26, 2017

That does seem a little silly, doesn't it?

The trivial solution to this is to serve the /api endpoint somewhere else, at say /jus-api instead. This would be easy and have minimal impact, since the jus core library doesn't use that endpoint outside of tests.

However, I don't know if any jus users have scripts that reference the existing api endpoint. I wouldn't want to introduce a breaking change for them. The other possibility is to make the api route configurable, through a command-line option probably. This has the advantage that it doesn't break anything, but it is less convenient for those who need the /api endpoint for something else.

@zeke , @wmhilton , any preference or insight here?

@t4t5
Copy link
Author

t4t5 commented Apr 26, 2017

I definitely don't think it's silly. :)

Of course it's easy to bypass the limitation if you're willing to settle for a less clean URL. I think however that the /api endpoint is one that many users might want for themselves, since it's very common for library docs to use it (examples: https://webpack.js.org/api, https://emberjs.com/api, https://pugjs.org/api/getting-started.html)

It's obviously not a deal breaker, but still a bit annoying. I agree that it would be nice to make the current api route configurable with some flag. Alternatively choose something less generic, if you want to emphasise the "convention over configuration" philosophy.

Just my ¢2!

@t4t5
Copy link
Author

t4t5 commented Apr 26, 2017

A third option could be that, when hitting the /API endpoint, Jus first checks whether the user has defined that route themselves in the file structure. If they have, then load that page. If not, just show the default JSON data as it does now.

Unless the /api endpoint is used internally by the library for some reason I think that could be a good solution too.

@elingerojo
Copy link
Member

@jdormit

...is to make the api route configurable

cli.js

. . .
process.env.JUS_API_ENDPOINT_NAME = args.api || args.a || 'api'
. . .

routes.js

. . .
  server.redirects = require('./redirects')(server)

  server.get('/' + process.env.JUS_API_ENDPOINT_NAME, (req, res) => {
    res.json(server.context)
  })

  server.get('/' + process.env.JUS_API_ENDPOINT_NAME + '/*', (req, res) => {
    var href = req.params[0].replace(/^files/, '')
    var page = server.context.pages.find(page => page.href === href)
. . .

Usage

$  jus serve  .  --api  jusapi

Rationale

  • Not include leading slash on the argument cause minimist does some path conversions (they could be solved by path.resolve(process.cwd()... but is to much for a simple configurable directory name)

Note:

@zeke @jdormit @wmhilton We need to do something about node-oniguruma breaking problem. I was unable to PR this simple api change (as neither #70 ) cause of failing test due to node-oniguruma dependency. Remeber master jus is not working for new installations with Node below 6.4 !

@jdormit
Copy link
Member

jdormit commented May 1, 2017

@elingerojo Oh man, I hadn't tried running jus with Node v4. Is there a reason you are using such an old version? Do you think it's important that we support such an old version?

@elingerojo
Copy link
Member

elingerojo commented May 1, 2017

@jdormit
Not really but...

...we are unable to do PR without CI-Travis and Appveyor complaints since the 20th of April because node-oniguruma

  • we should update dependencies accordingly...
  • ... change pagkage.json engines
  • ...and more important, change .travis.yml here and appveyor.yml here

ci-travis-error

I already submitted a PR to atom/node-oniguruma but they have not even commented about it.

As a side note, this "unable to PR" is dragging me on updating you guys about the #70 alpha-like-jus-app branch and the #61

@zeke
Copy link
Member

zeke commented May 1, 2017

Hey people sorry for the silence. I've been offline on parental leave for the last month. 🌴

I work with the atom team at GitHub, so I can take a look at that oniguruma PR and see if we can get a new version of it out. If it ends up being difficult, I would be open to releasing a new major version of jus that intentionally drops node 4 support.

I would also be open to allowing a command-line option to specify the API path. Or, as @t4t5 suggested, checking whether the user has defined the api route.

@t4t5 would you be interesting in submitting a PR to support that behavior?

@elingerojo
Copy link
Member

@zeke

I would also be open to allowing a command-line option to specify the API path.

I already have a local branch ready to PR but we are stuck with CI-Travis and Appveyor complaining for the node-oniguruma problem.

As soon as we can PR again, will submit the branch with the CLI api path option

@elingerojo
Copy link
Member

Just created an api-configurable-endpoint branch at my jus fork.

  • Added an optional CLI argument to specify the API endopoint instead of the default '/api'
  • Dumped Node 4 support in favor of Node 6 (just to get travis working 'cause node-oniguruma breaking problem)

Issues before doing a PR to jus/jus

  1. Should I include user facing instructions for the optional argument? I do not want to brake the "jus no configuration magic" but maybe an "Advanced Users" section could do the trick.

  2. Dumping Node 4 support should be discussed first

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

No branches or pull requests

4 participants