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)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.0b5
People
(Reporter: avarma, Assigned: warner)
References
Details
Attachments
(1 file)
7.38 KB,
patch
|
wbamberg
:
review+
irakli
:
feedback+
|
Details | Diff | Splinter Review |
See bug 614712 for more information.
Updated•13 years ago
|
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → 1.0b5
Updated•13 years ago
|
Assignee: nobody → warner-bugzilla
Assignee | ||
Comment 1•13 years ago
|
||
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?
Assignee | ||
Comment 2•13 years ago
|
||
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)
Comment 3•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #528253 -
Flags: review?(wbamberg) → review+
Comment 4•13 years ago
|
||
> Maybe I'll raise a separate bug to update the tutorial docs for 1.0b5. -> bug 652867
Assignee | ||
Comment 5•13 years ago
|
||
(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.
Comment 6•13 years ago
|
||
> > > > 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.
Assignee | ||
Comment 7•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
Landed in https://github.com/mozilla/addon-sdk/commit/bbf5363289f049456798268296e13019202469d6
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 9•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #528253 -
Flags: feedback?(rFobic) → feedback+
You need to log in
before you can comment on or make changes to this bug.
Description
•