Closed Bug 646293 Opened 13 years ago Closed 13 years ago

spell docs/ according to CommonJS rules (i.e. doc/)

Categories

(Add-on SDK Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: avarma, Assigned: warner)

References

Details

Attachments

(1 file)

See bug 614712 for more information.
Blocks: 614712
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → 1.0b5
Assignee: nobody → warner-bugzilla
Here's a quick patch which is supposed to tolerate documentation in either
"docs/" or "doc/" (but not both). Things to pay special attention to:

 - URLs (for the 'cfx docs' server) expose the doc/docs choice for each
   package, so some URLs will say e.g.
   ROOT/packages/addon-kit/docs/clipboard.html while others will say
   ROOT/packages/addon-kit/doc/clipboard.html .
 - package.json is *not* consulted: the code just looks for the existence of
   a docs/ directory, else defaults to doc/
 - the 'cfx sdocs' tarball should have the same URL choices as the server. I
   don't know if there are internal links that get messed up: in my quick
   testing, hosting the unpacked sdocs tarball locally didn't work before
   this patch, so I can't tell if I broke anything.
 - the 'cfx init' example now makes a doc/ directory. I didn't rename all the
   internal packages in this patch, but I could make another one to do that
   (and make everything consistent). Git can handle a rename like that pretty
   efficiently, but a simple flat diff would look huge.
Attachment #528253 - Flags: review?
Comment on attachment 528253 [details] [diff] [review]
prefer doc/ but still accept docs/

I'd like Irakli to think about this from a "what does CommonJS do" perspective, and Will to think about it from a "what about the sdocs tarball, and the docs server" perspective. Thanks!
Attachment #528253 - Flags: review?(wbamberg)
Attachment #528253 - Flags: review?
Attachment #528253 - Flags: feedback?(rFobic)
(In reply to comment #1)
> Created attachment 528253 [details] [diff] [review]
> prefer doc/ but still accept docs/
> 
> Here's a quick patch which is supposed to tolerate documentation in either
> "docs/" or "doc/" (but not both). Things to pay special attention to:
> 
>  - URLs (for the 'cfx docs' server) expose the doc/docs choice for each
>    package, so some URLs will say e.g.
>    ROOT/packages/addon-kit/docs/clipboard.html while others will say
>    ROOT/packages/addon-kit/doc/clipboard.html .

Yes. Links to modules in the sidebar, and in the 'package' pages (https://jetpack.mozillalabs.com/sdk/1.0b4/docs/packages/addon-kit/addon-kit.html) are generated, so will continue to work when a package moves docs from 'docs' to 'doc'. But internal links, such as those in the dev guide pages and other module doc pages, will need to be updated.

>  - package.json is *not* consulted: the code just looks for the existence of
>    a docs/ directory, else defaults to doc/
>  - the 'cfx sdocs' tarball should have the same URL choices as the server. I
>    don't know if there are internal links that get messed up: in my quick
>    testing, hosting the unpacked sdocs tarball locally didn't work before
>    this patch, so I can't tell if I broke anything.

That's a bit worrying. What command do you use, and what do you see?

I can build a functioning sdocs tarball, and it shows the behaviour I'd expect:

- with only this patch applied, the tarball is identical to the tarball without the patch, except for the changes to xpi.md neded to make doctests happy

- with the patch applied, and renaming the directory 'packages/addon-kit/docs' -> 'packages/addon-kit/doc', generated links to module pages (like the ones in the sidebar and in the package page) work fine, internal links are broken.

>  - the 'cfx init' example now makes a doc/ directory. I didn't rename all the
>    internal packages in this patch, but I could make another one to do that
>    (and make everything consistent). Git can handle a rename like that pretty
>    efficiently, but a simple flat diff would look huge.

Maybe I'll raise a separate bug to update the tutorial docs for 1.0b5.

Other than that, the code changes look good to me.
Attachment #528253 - Flags: review?(wbamberg) → review+
> Maybe I'll raise a separate bug to update the tutorial docs for 1.0b5.

-> bug 652867
(In reply to comment #3)
> (In reply to comment #1)
> > Created attachment 528253 [details] [diff] [review]
> > prefer doc/ but still accept docs/
> > 
> > Here's a quick patch which is supposed to tolerate documentation in either
> > "docs/" or "doc/" (but not both). Things to pay special attention to:
> > 
> >  - URLs (for the 'cfx docs' server) expose the doc/docs choice for each
> >    package, so some URLs will say e.g.
> >    ROOT/packages/addon-kit/docs/clipboard.html while others will say
> >    ROOT/packages/addon-kit/doc/clipboard.html .
> 
> Yes. Links to modules in the sidebar, and in the 'package' pages
> (https://jetpack.mozillalabs.com/sdk/1.0b4/docs/packages/addon-kit/addon-kit.html)
> are generated, so will continue to work when a package moves docs from
> 'docs' to 'doc'. But internal links, such as those in the dev guide pages
> and other module doc pages, will need to be updated.

Ah, I forgot that we've got internal links in the .md files themselves. But I
think we can ignore them unless/until we choose to modify the SDK's stdlib
packages to move from docs/ to doc/ . (I tested this stuff by moving
packages/addon-kit/docs to doc and running 'cfx docs', and the sidebare links
all worked, but I didn't run across any .md links).

> >  - the 'cfx sdocs' tarball should have the same URL choices as the
> >    server. I don't know if there are internal links that get messed up:
> >    in my quick testing, hosting the unpacked sdocs tarball locally didn't
> >    work before this patch, so I can't tell if I broke anything.
> 
> That's a bit worrying. What command do you use, and what do you see?

I used 'cfx sdocs' on trunk, unpacked the tarball, cd'ed into addon-sdk-docs/
, then started a web server with my usual "twistd -n web --path . --port
8080" (which ought to be mostly equivalent to 'mv addon-sdk-docs
~/public_html/' except for the prefix of the URL). The index.html rendered
fine. But following a module link like /packages/addon-kit/docs/hotkeys.html
results in a page which lost the CSS and jquery, so it looks weird and became
static. It looks like hotkeys.html has relative <link> hrefs like
href="css/base.css", and I'm not sure how those are supposed to work.

From the index page, following the "Reference" link takes me to an unstyled
static page at /dev-guide/module-development/reference.html . On that page I
click the "cuddlefish" link in the "Reference / api-utils" section, which
404s at it points to
/dev-guide/module-development/packages/api-utils/docs/cuddlefish.html .
Again, it looks like the cuddlefish.html link was relative: the source of
reference.html says href="packages/api-utils/docs/cuddlefish.html", which
makes me think that many of these .html pages are designed to be loaded into
a <div> on the front index page (from which the relative URLs would work),
and not as a standalone page.

> I can build a functioning sdocs tarball, and it shows the behaviour I'd
> expect:
> 
> - with the patch applied, and renaming the directory
>   'packages/addon-kit/docs' -> 'packages/addon-kit/doc', generated links to
>   module pages (like the ones in the sidebar and in the package page) work
>   fine, internal links are broken.

Ok, that sounds reasonable.

> Other than that, the code changes look good to me.

So I think what I need to know before landing is why my use of 'cfx sdocs' is
behaving so weirdly. Given that this patch didn't cause significant changes
in the tarballs you created, the patch is probably ok, but it'd be nice to
track down why a static tarball doesn't work correctly. But I can probably
still land it today.
> > 
> > That's a bit worrying. What command do you use, and what do you see?
> 
> I used 'cfx sdocs' on trunk, unpacked the tarball, cd'ed into addon-sdk-docs/
> , then started a web server with my usual "twistd -n web --path . --port
> 8080" (which ought to be mostly equivalent to 'mv addon-sdk-docs
> ~/public_html/' except for the prefix of the URL). The index.html rendered
> fine. But following a module link like /packages/addon-kit/docs/hotkeys.html
> results in a page which lost the CSS and jquery, so it looks weird and became
> static. It looks like hotkeys.html has relative <link> hrefs like
> href="css/base.css", and I'm not sure how those are supposed to work.

So the way this works these days is that all links, including links to stylesheets and scripts, are relative, and for sdocs the HTML <base> tag is used as the root for them. So when you run cfx sdocs you need to supply a new argument 'baseurl' which is the root directory for your web server. This gets written into the <base> tag for each page. For example:

cfx --baseurl https://jetpack.mozillalabs.com/sdk/1.0b4/docs/ sdocs

Could you try with the --baseurl argument, and see if that fixes the problem?

It's a pity, as it makes the tarball non-portable, but it's really a consequence of making the docs separate pages rather than all being the same page.
Aha. Yes, with --baseurl, the static tarball works great, both on trunk, and with-the-patch and rename-addon-kit/docs-to-addon-kit/doc .

Ok, I think I'll land this now and keep an ear open for concerns from Irakli.
Landed in https://github.com/mozilla/addon-sdk/commit/bbf5363289f049456798268296e13019202469d6
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
In commonjs you can specify dir for docs via `package.json`'s `directories.doc` which defaults to `"./doc"` if not present. I think eventually supporting that is a better alternative, but this is good step forward I think.
Attachment #528253 - Flags: feedback?(rFobic) → feedback+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: