Allow using of non-word characters in [import] tag #52
Conversation
|
Please take a look (I've missed by a day, since there are a lot to read from different packages). There are still two issues to solve, so please advice whether I should ask them here or to put them in another PR. BTW, some GitBook dependencies appears to be broken in the CI. :-( |
|
Found that the build failure is due to a bug in gitbook-cli 2.3.1. See GitbookIO/gitbook-cli#68 for the bug report. |
src/parser.js
Outdated
| @@ -3,13 +3,25 @@ | |||
| const path = require("path"); | |||
| const Handlebars = require("handlebars"); | |||
| const logger = require("winston-color"); | |||
| import { defaultKeyValueMap, initOptions, checkMapTypes, convertValue } from "./options.js"; | |||
| const common = require("commonmark/lib/common.js"); | |||
There was a problem hiding this comment.
Does commonmark export this? ( Is it a undocument API? )
We want to avoid to use direct require.
There was a problem hiding this comment.
No it is not documented, nor exported by commonmark package.
I'll copy the implementation to our own code (and the license too - which appears to be 2-clause BSD).
src/parser.js
Outdated
| let match = ""; | ||
| while ((match = expressionRegExp.exec(label))) { | ||
| let key = match[1]; | ||
| if (["include", "import"].includes(key)) { |
There was a problem hiding this comment.
Node.js v4 does not support Array#inclues
http://node.green/#ES2016-features-Array-prototype-includes-Array-prototype-includes
We want to support Node.js LTS as possible.
Can you use indexOf or add ponyfill or polyfill?
There was a problem hiding this comment.
Shouldn't this be handled by babel instead of by us? I mean, you can use "babel-preset-env", and specify the node version in .babelrc
BTW, I was told that the gitbook server used node 5.1.*. But recently the broken gitbook-cli 2.3.1 moved to node LTS (currentlt v6).
There was a problem hiding this comment.
Shouldn't this be handled by babel instead of by us?
No. Babel only transpile syntax by default.
- https://babeljs.io/docs/usage/polyfill/
- https://github.com/babel/babel-preset-env#usebuiltins
- Need to add
import "babel-polyfill";
- Need to add
I think that this PR fix a bug - it is a patch version update.
Dropping Node.js 4.x make breaking change - it is a major version update.
There was a problem hiding this comment.
Thanks for your clarification! Done.
test/parser-test.js
Outdated
| assert.equal(result, " marker0 , marker1 "); | ||
|
|
||
| result = parseValue('"~invalid~"', "string", "marker"); | ||
| assert.equal(result, undefined); |
src/parser.js
Outdated
| export function parseValue(value, type, key) { | ||
| switch (type) { | ||
| case "string": | ||
| value = common.unescapeString(value.substring(1, value.length - 1)); |
There was a problem hiding this comment.
const unescapedValue = ... is better.
|
I think it's better that adding pattern test to https://github.com/azu/gitbook-plugin-include-codeblock/tree/master/test/patterns |
|
Thanks for pointing out at the pattern test. I'll add a few there. |
|
The new patch is finally prepared. Please take a look. To resolve failures in CI, I've blacklisted "gitbook-cli" 2.3.1 in the example/*/package.json. |
|
|
||
| src/unescape-string.js is extracted from https://github.com/jgm/commonmark.js: | ||
|
|
||
| Copyright (c) 2014, John MacFarlane |
|
LGTM |
|
@lwchkg Thanks! |
|
Release 3.1.1 |
title:"edit:true"or the like. (edit should not be set to true in this case).Known problems:
title:"he's good"properly. In this patch the title we be set to "he".\.