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

Add support for file: syntax #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add support for file: syntax #20

wants to merge 3 commits into from

Conversation

leoj3n
Copy link
Contributor

@leoj3n leoj3n commented Apr 9, 2017

Some commits of this PR depend on bit-docs/bit-docs#26 being merged.

Alternative is to use __dirname in the plugins' bit-docs.js: bit-docs/bit-docs#25


Useful if you want to have bit-docs use local copies of plugins:

  "bit-docs": {
    "dependencies": {
      "bit-docs-dev": "bit-docs/bit-docs-dev",
      "bit-docs-generate-html": "file:docs/modules/bit-docs-generate-html",
      "bit-docs-glob-finder": "bit-docs/bit-docs-glob-finder",
      "bit-docs-html-toc": "bit-docs/bit-docs-html-toc",
      "bit-docs-js": "bit-docs/bit-docs-js#remove-depend",
      "bit-docs-prettify": "file:docs/modules/bit-docs-prettify",
      "bit-docs-html-highlight-line": "bit-docs/bit-docs-html-highlight-line",
      "bit-docs-process-mustache": "bit-docs/bit-docs-process-mustache",
      "bit-docs-tag-demo": "bit-docs/bit-docs-tag-demo",
      "bit-docs-tag-sourceref": "bit-docs/bit-docs-tag-sourceref"
    },

Notice that the file: dependencies will not pull from GitHub like the others, but now will instead use the local copy thru and thru. In this case, items in docs/modules are git submodules.


The changes in this PR are necessary specifically for working with local file: plugins that register themselves to the html hook (a hook that is handled by bit-docs-generate-html).

Such a plugins are:

Without this PR and when using file: to include them, any changes to files such as:

Won't come through, because currently bit-docs-generate-html will download from remote npm.

Not needed now that bit-docs makes all file: paths absolute:

bit-docs/bit-docs#26
for(var depName in siteConfig.html.dependencies) {
var possibleFilePath = siteConfig.dependencies[depName];

if (_.startsWith(possibleFilePath,'file:')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to use Lodash instead of startsWith or indexOf?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I don't understand what you're asking. I literally wanted to know what the code is saying: Does it start with this string or not, and lodash was already required in the file. Otherwise I would have used indexOf.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should’ve been more clear; any reason not to use the native String APIs here? Not a big deal, just asking if there’s a reason to use Lodash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability it's good.

@leoj3n
Copy link
Contributor Author

leoj3n commented Apr 16, 2017

Is there a reason that plugins registering themselves with the html hook (subsequently handled by bit-docs-generate-html) do so by specifying themselves as remote dependencies? If a plugin is registering itself, it is therefore already downloaded, and so why not specify the local copy? Like:

bit-docs/bit-docs#25 (comment)

If plugins were expected to use __dirname with html like that, instead of version number like they do currently, then this PR with the convoluted string check for file: would not be necessary, and regardless of if the plugin is local or remote, it will work just the same without having an unnecessary call out to remote npm and subsequent downloads that can slow things down a lot (so in addition to supporting local plugin development, usinge __dirname would speed things up regardless).

If it doesn't break anything, I suggest we change the plugins that need to load front-end JS/Less through bit-docs-generate-html's html hook (like bit-docs-prettify and bit-docs-html-highlight-line) to using the absolute __dirname path instead of their remote version string.

@leoj3n
Copy link
Contributor Author

leoj3n commented Apr 16, 2017

Before this PR, the file:

node_modules/bit-docs/lib/configure/node_modules/bit-docs-generate-html/site/static/build/917c51373c902dc4a003c7fd949774c5/package.json

Would have dependencies like:

{
  "name": "bit-docs-site",
  "version": "0.0.1",
  "description": "A site to be built",
  "main": "static.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "Bitovi",
  "license": "MIT",
  "dependencies": {
    "steal": "0.16.X",
    "bit-docs-html-toc": "0.6.2",
    "bit-docs-prettify": "0.1.1",
    "bit-docs-html-highlight-line": "0.2.3",
    "bit-docs-tag-demo": "0.3.0"
  },
  "system": {
    "npmAlgorithm": "flat"
  }
}

After this PR (or, alternatively, using __dirname for the plugins registering on the html hook):

node_modules/bit-docs/lib/configure/node_modules/bit-docs-generate-html/site/static/build/917c51373c902dc4a003c7fd949774c5/package.json:

{
  "name": "bit-docs-site",
  "version": "0.0.1",
  "description": "A site to be built",
  "main": "static.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "Bitovi",
  "license": "MIT",
  "dependencies": {
    "steal": "0.16.X",
    "bit-docs-html-toc": "file:\/Users\/leoj\/bitovi\/,+\/bd\/gm\/bit-docs-website\/docs\/modules\/bit-docs-html-toc",
    "bit-docs-prettify": "file:\/Users\/leoj\/bitovi\/,+\/bd\/gm\/bit-docs-website\/docs\/modules\/bit-docs-prettify",
    "bit-docs-html-highlight-line": "file:\/Users\/leoj\/bitovi\/,+\/bd\/gm\/bit-docs-website\/docs\/modules\/bit-docs-html-highlight-line",
    "bit-docs-tag-demo": "file:\/Users\/leoj\/bitovi\/,+\/bd\/gm\/bit-docs-website\/docs\/modules\/bit-docs-tag-demo"
  },
  "system": {
    "npmAlgorithm": "flat"
  }
}

Note: We would probably want to use file: like 'file:' + __dirname in the plugin's bit-docs.js (even though absolute paths without file: seem to work fine, using file: meets the npm spec).

@leoj3n
Copy link
Contributor Author

leoj3n commented Apr 20, 2017

Regardless of supporting file: in bit-docs/bit-docs (to do so, bit-docs would need to always make the file: path be absolute as proposed in that PR bit-docs/bit-docs#26), bit-docs-generate-html should support npm install ../path/to/local/copy and npm link my-plugin.

Currently, for getting an arbitrary plugin's front-end code into the generated website, bit-docs-generate-html will npm install from whatever version string is assigned to that key on the dependencies object passed into the html hook.

The problem is that all plugins currently specify an actual version number like 0.1.0 which causes bit-docs-generate-html to always download from remote npm, which is an issue when attempting to debug or develop locally.

One solution is to have the plugin specify its version string as the plugin's __dirname, like:

bit-docs/bit-docs-js#4
bit-docs/bit-docs-prettify#8

However, there may be a better option, as pointed out by @justinbmeyer, that involves passing the plugin name and version to the plugin's bit-docs.js from the bit-docs require here:

https://github.com/bit-docs/bit-docs/blob/master/lib/configure/add-package-configuration.js#L8

That would essentially be the same as the __dirname option, just a little cleaner.

@leoj3n
Copy link
Contributor Author

leoj3n commented Apr 20, 2017

To use npm install or npm link (without support for file:), people would have to cd to their-website/node_modules/bit-docs/lib/configure before running link or install. So, adding __dirname for this case is seeming not so important because of the kludge, and as we talked about @justinbmeyer, it would be better do this kind of integrated test within a make-example.js harness within bit-docs-generate-html itself.

However, I have thought of another and very important reason to support __dirname in the effort to circumvent remote npm. It doesn't have to do with bad integration test paradigm nor with speeding things up by avoiding the repetitive npm download (though of course that still exists).

It has to do with the case when someone specifies a GitHub repository as the version string in the bit-docs.dependencies. Right now if you do so, bit-docs will correctly get the GitHub version of the plugin, but subsequently bit-docs-generate-html will just get whatever is set as version on the package.json, and try to download that from remote npm.

So, for a website package.json like:

  "bit-docs": {
    "dependencies": {
      "bit-docs-dev": "bit-docs/bit-docs-dev",
      "bit-docs-generate-html": "bit-docs/bit-docs-generate-html",
      "bit-docs-glob-finder": "bit-docs/bit-docs-glob-finder",
      "my-cool-plugin": "dude/my-cool-plugin",
      "bit-docs-js": "bit-docs/bit-docs-js"
    },

We most definitely need to use something like __dirname for plugins registering to html hook.

@leoj3n
Copy link
Contributor Author

leoj3n commented Sep 21, 2017

Note that in conversation I've had with Justin he agrees this is a usability win for end users, but it isn't "correct". Using npm install instead is more correct because in order for that you have to be all setup with npm and already using it for that plugin, which is more correct but perhaps less intuitive for newer users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants