ember-learn / ember-cli-addon-docs

Easy, beautiful docs for your OSS Ember addons
https://ember-learn.github.io/ember-cli-addon-docs
MIT License
176 stars 143 forks source link

Allow for overwriting base URL #133

Closed alexlafroscia closed 6 years ago

alexlafroscia commented 6 years ago

I'm trying to use ember-cli-addon-docs to generate the documentation for a project that's within a Lerna project. I'm running into issues based on how the "production" deployment build makes assumptions around what the URL is going to be.

In my case, the repo is called testdouble-qunit but the Ember add-on's name is ember-cli-testdouble-qunit. It's all fine locally, but the generated production build makes an assumption that the URL should be /ember-cli-testdouble-qunit/VERSON/ for any given version.

I think it would be great if the configuration file provided a hook that could override this behavior.

I'm going to look into getting a fix together for this myself, but figured I would open an issue first.

dfreeman commented 6 years ago

@alexlafroscia Thanks for opening this! I thought we already had an issue for it, but it looks like we may have just discussed it in slack at some point instead.

For the specific case you're talking about here where the name just doesn't align, things should be pretty easy to fix. More generally, though, we have a structural problem where we assume a single leading segment in the URL, which won't always be true (e.g. if you have a custom domain set up for your site, like https://www.ember-cli-mirage.com).

If you'd be up for/interested in solving the more general problem, I'm happy to point you at where I think it's impacting us and spitball ideas for how to deal with it.

alexlafroscia commented 6 years ago

That sounds good too! Let's solve it the "right way". I link the idea that the latest "version" could actually just be the root URL (in my example, alexlafroscia.com/testdouble-qunit rather than alexlafroscia.com/testdouble-qunit/latest).

What are your thoughts on this so far?

alexlafroscia commented 6 years ago

Just for gathering notes, I found that the main issue in my case is that

  1. The project URL is determined by the addon's name in index.js
  2. The addon's name in index.js determines how the addon generates the docs/__NAME__.json file
  3. The addon, at runtime, uses the package.json name field to fetch the docs/__NAME__.json

So if everything is the same, it's no problem. However, if you change the name of the addon in index.js to adjust the URL, you've now changed the name of the .json file in a way that the runtime can't play nicely with.

dfreeman commented 6 years ago

So if everything is the same, it's no problem. However, if you change the name of the addon in index.js to adjust the URL, you've now changed the name of the .json file in a way that the runtime can't play nicely with.

Ember CLI is actually about to (or maybe already did as of 3.0?) start issuing a warning if your name in index.js doesn't match the name in package.json, so while it would probably be nice if we were consistent about where we sourced that information, ultimately I think it won't be the end of the world if we expect them to be the same.

I link the idea that the latest "version" could actually just be the root URL (in my example, alexlafroscia.com/testdouble-qunit rather than alexlafroscia.com/testdouble-qunit/latest).

I think @samselikoff had done some research that motivated the use of /latest in the URL for external reasons, but even putting that aside, it winds up complicating things for us a fair bit on the deploy side if we drop that segment, for a couple reasons:

dfreeman commented 6 years ago

That second bullet point gets at the places that I know of where we assume a single segment right now:

I think if we were to expose a getStaticRootPath() hook (or something along those lines) in config/addon-docs.js, we could use that information to make both the service and the 404.html more intelligent about their assumptions.

alexlafroscia commented 6 years ago

Ember CLI is actually about to (or maybe already did as of 3.0?) start issuing a warning if your name in index.js doesn't match the name in package.json, so while it would probably be nice if we were consistent about where we sourced that information, ultimately I think it won't be the end of the world if we expect them to be the same.

Totally. I was just trying to change them because it was the only way to override the URL that the application is mounted at. I don't want them to be different.

I started to look at something just like that you mentioned (a function in config/addon-docs.js) a few days ago. I can take a look more seriously this weekend to determine how that would need to work.

dfreeman commented 6 years ago

Following up from discussion in Slack, there are two main benefits (that I'm aware of) to having the /latest segment.

The first is that it simplifies deploys, since we can always safely blow away the contents of the target build directory. Without the dedicated directory, updating the 'latest' deploy of the app means carefully deleting most of the contents of the root while not disturbing any other deployed versions. This is totally doable (and in fact we did something very similar in an earlier cut of the deployment logic), but it's a matter of someone investing the effort into building it.

The second benefit is that it makes the 'latest' deploy symmetric with all other deployed copies of the app with respect to its URL structure. We have a couple of places where we inspect the active URL to determine where we are relative to the rest of the world. One is in the project-version service, and the other is in the 404.html that allows us to fake having fancy routing on Github pages. Both of those places will need to become smarter to solve the original issue documented here anyway, so the main addition to that which we'd need (as @ef4 called out) would be to reserve a URL segment for all other versions to live under.

Both of the benefits of /latest outlined above are essentially only advantageous to us as maintainers of addon-docs: they're simplifications that eliminate some code and complexity that we'd otherwise have to deal with. It seems to me that from the documentation consumer's perspective, there's no advantage to /latest at all, and it's nicer not to have it.

If we were to move forward with eliminating /latest, though, it would mean restricting some freedom we currently provide, and there would also be a potentially bumpy migration for any early-adopters who've already got deployed revisions out there.

I don't see either of those as necessarily being blockers, but it does mean if we're going to make this change it should likely be sooner rather than later.

@samselikoff @pzuraq any strong opinions one way or the other?

dfreeman commented 6 years ago

I'll also note that all the /latest business can be resolved independently of the actual contents of @alexlafroscia's original issue here. The two things are tricky for some overlapping reasons, but they don't need to be handled all at once.

alexlafroscia commented 6 years ago

I ended up solving my problem with this change:

https://github.com/alexlafroscia/testdouble-qunit/blob/a242c83fcf5b3e737a4f2913904443db6c7bea68/packages/ember-cli-testdouble-qunit/config/deploy.js#L8

Between updating to 0.4.0 and then adding that explicit repo reference, it started building the correct URLs again.

I did implement a really simple version of getBasePath though, so I can still PR that if it's wanted.