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

Bad handling of block arguments #934

Closed
piranna opened this issue Sep 17, 2015 · 6 comments
Closed

Bad handling of block arguments #934

piranna opened this issue Sep 17, 2015 · 6 comments

Comments

@piranna
Copy link

piranna commented Sep 17, 2015

args and kwargs fields of block object are incorrectly filled. When using a single argument like {% mermaid "user.mmd" %}{% endmermaid %}, the block object get filled as

{ body: '', args: [], kwargs: 'user.mmd', blocks: [] }

and when using two arguments like {% mermaid "user.mmd", "2" %}{% endmermaid %} it gets filled as

{ body: '', args: [ 'user.mmd' ], kwargs: '2', blocks: [] }

According to the docs, positional arguments should be filled in args always, that makes sense. Keyword arguments ({% mermaid src="user.mmd" %}{% endmermaid %}) seems to works as expected:

{ body: '',
  args: [],
  kwargs: { src: 'user.mmd', __keywords: true },
  blocks: [] }

By the way, I think that __keywords field is mostly useless...

@SamyPesse
Copy link
Member

That's weird, there are tests for all of this: https://github.com/GitbookIO/gitbook/blob/master/test/plugins.js#L195

What version of node are you using ? Are the gitbook unit tests passing for you ?

@SamyPesse
Copy link
Member

I've added more tests and it's still passing correctly (f5afbc9).

@piranna
Copy link
Author

piranna commented Sep 18, 2015

What version of node are you using ?

v0.12.7

Are the gitbook unit tests passing for you ?

How can I be able to know what version of GitBook is downloading the gitbook-cli?

[OffTopic]: instead of define the version with a field on book.json (or as an alternative to it), why don't instead set gitbook module as dependency of the book package.json file? And the same with the GitBook plugins, they give problems for the ones not published on npm or when using development versions from GitHub...

@SamyPesse
Copy link
Member

Run gitbook versions:print

Can you run the unit test and tell me if there are passing ? I can't reproduce your issue.

To run unit tests:

$ git clone https://github.com/GitbookIO/gitbook.git
$ cd gitbook
$ npm i
$ npm test

99% people using GitBook are not node developers, I made the choice of using only one file to define the configuration book.json instead of two (package.json and book.json), and avoid explaining to all how to write a coherent package.json. But gitbook-cli can easily use on top priority the gitbook in node_modules so that you can a package.json if you want.

For plugins, using book.json to list plugins dependencies allow to resolve by default to the latest supported version (engine field in the package.json). Plugins support evolved a lot with version of GitBook.

For example if a book is built with GitBook 2.0.0, and use a plugin test, but this plugin evolves to supporte recent API of GitBook 2.3.0 ("engines": { "gitbook": ">=2.3.0" } }; the book will stop building because the latest download version (by default) will not work with version 2.0.0.
Most of the authors don't want to spend time checking versions of plugins, they just want to specify a list of plugins and it should work as expected; in that case GitBook do the job of resolving to the correct version.

This is just choices that I made, bring simplicity to the 90% of our authors, maybe it's a mistake, but I'm currently convince that it's a better idea to not see GitBook as "a node.js library", but as a complete/independent toolchain; even if that hurts some Node.js developers like you.

@piranna
Copy link
Author

piranna commented Sep 19, 2015

Run gitbook versions:print

[piranna@Mabuk:~/Universidad Rey JuanCarlos/pfc]
 (master) > gitbook versions:print
Current version is 2.1.0

Can you run the unit test and tell me if there are passing ? I can't reproduce your issue.

Initially they were not passing because I haven't had svgexport installed globally (I have it set as a dependency of my book), but after installing it globally tests have fully passed.

99% people using GitBook are not node developers, I made the choice of using only one file to define the configuration book.json instead of two (package.json and book.json), and avoid explaining to all how to write a coherent package.json.

I agree, and don't believe it's a bad thing, but being a node.js app it will be inevitable they will find some node.js spikes... That's from the perspective that generating the books manually it's a first-class use case, or if it's only the GitBook editor (both desktop app or cloud one). In that last case your approach it's correct. In my case, I've "fixed" it by adding a postinstall, pdf and serve tasks to the scripts sections of my package.json, so for readers to generate the book is so simple as

npm install
npm run pdf

But gitbook-cli can easily use on top priority the gitbook in node_modules so that you can a package.json if you want.

book.json is not the problem, but allowing to use package.json fields by default and maybe define there a gitbook section as an alternative to book.json would be a cool and easy improvement. A better integration with Node.js environment and conventions could come later from here.

For plugins, using book.json to list plugins dependencies allow to resolve by default to the latest supported version (engine field in the package.json). Plugins support evolved a lot with version of GitBook.

For example if a book is built with GitBook 2.0.0, and use a plugin test, but this plugin evolves to supporte recent API of GitBook 2.3.0 ("engines": { "gitbook": ">=2.3.0" } }; the book will stop building because the latest download version (by default) will not work with version 2.0.0.
Most of the authors don't want to spend time checking versions of plugins, they just want to specify a list of plugins and it should work as expected; in that case GitBook do the job of resolving to the correct version.

If I understood it correctly, by setting gitbook modules as a dependency just as grunt does, npm already solve this by using the peerDependencies on the plugin...

This is just choices that I made, bring simplicity to the 90% of our authors, maybe it's a mistake, but I'm currently convince that it's a better idea to not see GitBook as "a node.js library", but as a complete/independent toolchain; even if that hurts some Node.js developers like you.

I don't think it's a bad decission, but definitely they are not incompatibles, we already can have gitbook as a "node.js library" while at the same time gitbook-cli is a tool to help make things easier, just only need to change some little internal bits to use current npm standard and conventions instead of make our new ones :-) By giving a higher priority to package.json over book.json when reading info and dependencies (like not try to download a module if it's already installed) this would heavily improve without making it more dificult to newcomers or break compatibility :-)

If it's of your interest, that's the package.json file of my book:

{
  "name": "NodeOS",
  "description": "Sistema operativo ligero para Node.js",
  "author": "Jesús Leganés Combarro 'piranna' <piranna@gmail.com>",
  "version": "0.0.0",
  "scripts": {
    "pdf": "gitbook pdf . NodeOS.pdf",
    "postinstall": "gitbook install && npm install piranna/gitbook-plugin-mermaid#patch-1",
    "serve": "gitbook serve ."
  },
  "repository": {
    "type": "git",
    "url": "https://github.com/piranna/pfc.git"
  },
  "bugs": {
    "url": "https://github.com/piranna/pfc/issues"
  },
  "homepage": "https://github.com/piranna/pfc",
  "devDependencies": {
    "gitbook-cli": "^0.3.6",
    "svgexport": "^0.2.5"
  },
  "dependencies": {
    "gitbook-plugin-autocover": "GitbookIO/plugin-autocover",
    "gitbook-plugin-mermaid": "piranna/gitbook-plugin-mermaid#patch-1"
  }
}

@piranna
Copy link
Author

piranna commented Sep 19, 2015

By the way, the npm install on the postinstall script is because gitbook insist on overwrite the one on my dependencies with the one from npm, that's outdated...

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

3 participants