azu / gitbook-plugin-include-codeblock

GitBook plugin for including file
Other
40 stars 25 forks source link

Parser refactor #43

Closed gdolle closed 7 years ago

gdolle commented 7 years ago

Major refactoring for the parser. Breaking changes (3.1.0 ?)

Changelog

It should work far better with this PR :)

I've been noticed that ace plugin might crash after a page reload in previous version, but I couldn't reproduce that problem. However, these changes fix some bugs and "might" be fix for that behaviour, we will see.

NB: It might be interesting to have a look to async operation in the future to optimize the code.

@azu waiting for your review, you'll have some work ;)

azu commented 7 years ago

Oh, large diff.. I'll look it lator. Can you separate refactor PR and feature PR if possible.

gdolle commented 7 years ago

@azu Actually, features are minors and deeply dependent of the of refactor. It'd be quite difficult to separate now.

Would it be ok to keep it as one PR ?

The related features changes are

(I added comments on file changes to help)

azu commented 7 years ago

@gdolle fmm, I want to separate big PR to smll PRs by following reasons.

Add Url support #42 (sync http request using sync-request modules + check url valid-url)

Sync request is simple solution, but it is not correct solution. Because GitBook support async hook.

(Not actuall code)

// THIS IS IMAGINE
module.exports = {
    hooks: {
        "page:before": function(page) {
            var options = this.options.pluginsConfig["include-codeblock"];
            var pageDir = path.dirname(page.rawPath);
            var parseResults = parse(page.content, pageDir, options);
            var promises = results.map(result => {
                return requestUrlAndCreateReplaceContent(result).then(newContent => {
                     replaceContent(page, newContent);
                });
            });
            return Promise.all(promises); // GitBook support async by returing promise
        }
    }
};

and

Immutable.js is overlarge for this project. I recommened that use ES2015 Map + Poilyfill insteadof immutable.js.


Of course, Welcome to bugfix, but the bugfix should not contains other feature. I hesitate to merge the bugfix PR.

Sorry, slow response.

gdolle commented 7 years ago

Does ES2015 Map guaranty const Map values ?

Actually refactor parser:

  1. immutable Map for read only (more robust to my opinion)
  2. handle key-val types (boolean, string, number). The "bug fix" is a consequence of the parser refactor (maybe my single comment is error prone) it's not really a feature.

I agree for the url feature, it is from far not perfect. I'll separate it in another PR.

azu commented 7 years ago

(Currently, this project mixed codeing style, We should apply ESLint #39)

gdolle commented 7 years ago

@azu I updated the code.

I didn't apply yet Eslint, I'll have a look into that.

gdolle commented 7 years ago

@azu I added eslint config following #39 and fix current code following the convention. I also added a "check" script which test eslint on src/ and test/. This script is executed in travis, therefore we can see if something is not good with respect to the coding style. Let me know if it's ok for you like that. ;)

gdolle commented 7 years ago

Sorry for the late reply. I updated the code relatively to your last review

azu commented 7 years ago

@gdolle Thanks for great work!