-
Notifications
You must be signed in to change notification settings - Fork 124
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
Adopt ES6 class syntax #979
Conversation
@yamgent help wanted: could you please explain a bit on what |
From the jsdoc of /**
* Creates a function that delays invoking `func` until after `wait` milliseconds have elapsed
* and the running `func` has resolved/rejected. What
I find that quite powerful; it solves #155. The original PR MarkBind/markbind-cli#31 contains the initial discussion.
What was ESLint complaining about? |
Thanks for the explanation! That makes a lot of sense now.
If I write like
as it expects the method to be a function. |
If this line was directly within the
ES6 classes don't allow 'assignments' inside the class template, I think |
Well this is quite different as a new Just retain the original
after the class declaration for now? Some time after this proposal is merged https://github.com/tc39/proposal-class-fields we can revisit this. |
My bad 😅 , good catch Sounds good |
It's also possible to use this solution, but it may complicate things, so I might as well stick to the prototype version for |
Nah, that's not a solution since we're targeting ES6 which does not allow class properties. Class properties aren't in any ES specification yet, so even though it works on node 12 it's possible that they take it out in a future version. |
If there's no rejection on adopting the ES6 class syntax I'll start working on refactoring other classes then =) |
Ok please go ahead. 👍 |
Extracted out utility functions into pageUtil.js for better consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments applies to all the files:
add them as static methods in respective classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly okay, just minor syntax changes to ES6 classes and not much code changes.
- adopt ES6 syntax for unique(array) - remove redundant Site method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do realize that the blank lines are being removed as well. Were all the blank lines removed, or was it manually done to clean up unnecessary blank lines? Just wondering as functions in MarkBind are quite long, so those blank lines may aid in easier reading of the code.
const Promise = require('bluebird'); | ||
const url = require('url'); | ||
const pathIsInside = require('path-is-inside'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The imports should be in alphabetical order.
src/lib/markbind/src/parser.js
Outdated
IMPORTED_VARIABLE_PREFIX, | ||
BOILERPLATE_FOLDER_NAME, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideal to have the constants in alphabetical order too.
ESLint will complain if there are more blank lines:
I will try to document all functions for better readability in a future PR. |
to alphabetical order
Btw GitHub is complaining about conflicting files, you will have to resolve it. You can also squash the commits for this PR, we won't need the separate commits for now. |
@openorclose I reverted the global variables in |
# Conflicts: # src/Page.js # src/Site.js
LGTM, it shouldn't cause any issues with plugin live reloading if live reload already works. There's a few pumls at the bottom of the test site you can try editing to test plugin live reloading though On another note I noticed you made quite a few style changes to existing method calls ( newlines between chained method calls ). Was this due to a conflict with an eslint rule from using classes? It might be better to make such changes in a seperate PR to make the diff easier to compare otherwise! |
Sorry there's another merge conflict due to a recent merge of the big feature #944. Would need you to rebase again. Once your PR is rebased and in order, I will prioritize your PR to be merged next. |
@yamgent I (again) tried my best effort to merge this conflict... can @crphang do some quality assurance on this merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good work on the migration 🎉. LGTM in general, don't see any issues with resolution of merge conflicts.
const filePath = element.attribs[ATTRIB_INCLUDE_PATH]; | ||
const fileBase = calculateNewBaseUrls(filePath, config.rootPath, config.baseUrlMap); | ||
if (!fileBase.relative) { | ||
return pa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think it should be resolved in this PR, but can this be declared in the constructor?
What do you think @yamgent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will probably take a bit of work to move it to the constructor, since it is likely that the original author intentionally makes it globally scoped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yes let's leave it to another PR.
eb5d71e
to
ab17775
Compare
ab17775
to
1d544b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little late, but, I noticed there are quite a few strange newlines-after calls to $(...)
that makes the code harder to read. Subjective in some cases, but rather awkward in others.
Should we keep to a minimum of three method calls before indenting with newlines? a().b().c()
this.keywords = {}; | ||
// Collect headings and keywords | ||
this.collectHeadingsAndKeywordsInContent($(`#${CONTENT_WRAPPER_ID}`) | ||
.html(), null, false, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
$('modal') | ||
.remove(); | ||
$('panel') | ||
.remove(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
*/ | ||
linkKeywordToHeading($, keyword, heading) { | ||
const headingId = $(heading) | ||
.attr('id'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
this.keywords[headingId] = []; | ||
} | ||
this.keywords[headingId].push($(keyword) | ||
.text()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
I believe this is mostly due to WebStorm’s auto formatting. (It turns out
to be a bit unreliable sometimes, but I think it’s well justified as I was
doing such extensive refactoring. Will try to avoid that in the future.).
That being said, I will create another PR to fix the JSDoc issues and
formatting issues. Thanks!
On Fri, 31 Jan 2020 at 1:23 AM, Ang Ze Yu ***@***.***> wrote:
***@***.**** commented on this pull request.
A little late, but, I noticed there are quite a few strange newlines-after
calls to $(...) that makes the code harder to read. Subjective in some
cases, but rather awkward in others.
Should we keep to a minimum of three method calls before indenting with
newlines? a().b().c()
------------------------------
In src/Page.js
<#979 (comment)>:
> -Page.prototype.collectNavigableHeadings = function (content) {
- const { pageNav } = this.frontMatter;
- const elementSelector = this.generateElementSelectorForPageNav(pageNav);
- if (elementSelector === undefined) {
- return;
+ /**
+ * Records headings and keywords inside rendered page into this.headings and this.keywords respectively
+ */
+ collectHeadingsAndKeywords() {
+ const $ = cheerio.load(fs.readFileSync(this.resultPath));
+ // Re-initialise objects in the event of Site.regenerateAffectedPages
+ this.headings = {};
+ this.keywords = {};
+ // Collect headings and keywords
+ this.collectHeadingsAndKeywordsInContent($(`#${CONTENT_WRAPPER_ID}`)
+ .html(), null, false, []);
.
------------------------------
In src/Page.js
<#979 (comment)>:
> });
- }
- $('.keyword').each((i, keyword) => {
- let closestHeading = getClosestHeading($, headingsSelector, keyword);
- if (excludeHeadings || !closestHeading) {
- if (!lastHeading) {
- logger.warn(`Missing heading for keyword: ${$(keyword).text()}`);
- return;
- }
- closestHeading = lastHeading;
+ $ = cheerio.load(content);
+ if (this.headingIndexingLevel > 0) {
+ $('modal')
+ .remove();
+ $('panel')
+ .remove();
.
------------------------------
In src/Page.js
<#979 (comment)>:
> - * @param keyword to link
- * @param heading to link
- */
-Page.prototype.linkKeywordToHeading = function ($, keyword, heading) {
- const headingId = $(heading).attr('id');
- if (!(headingId in this.keywords)) {
- this.keywords[headingId] = [];
+ /**
+ * Links a keyword to a heading
+ * @param $ a Cheerio object
+ * @param keyword to link
+ * @param heading to link
+ */
+ linkKeywordToHeading($, keyword, heading) {
+ const headingId = $(heading)
+ .attr('id');
.
------------------------------
In src/Page.js
<#979 (comment)>:
> - if (!(headingId in this.keywords)) {
- this.keywords[headingId] = [];
+ /**
+ * Links a keyword to a heading
+ * @param $ a Cheerio object
+ * @param keyword to link
+ * @param heading to link
+ */
+ linkKeywordToHeading($, keyword, heading) {
+ const headingId = $(heading)
+ .attr('id');
+ if (!(headingId in this.keywords)) {
+ this.keywords[headingId] = [];
+ }
+ this.keywords[headingId].push($(keyword)
+ .text());
.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#979?email_source=notifications&email_token=AG5DZVXWBBLDCGEN5WD7PT3RAMEHRA5CNFSM4KGQ2LYKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTV5TTY#pullrequestreview-351001039>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG5DZVRGVWFTVOUMNKJPIHDRAMEHRANCNFSM4KGQ2LYA>
.
--
Regards,
Tan Yuanhong
|
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature
What is the rationale for this request?
ES6 class syntax is more beginner-friendly, and clearer to read.
What changes did you make? (Give an overview)
Site.js
,Page.js
andparser.js
withclass
keyword.Provide some example code that this change will affect:
na
Is there anything you'd like reviewers to focus on?
na
Testing instructions:
npm run test
Proposed commit message: (wrap lines at 72 characters)
Adopt ES6 class syntax for Site, Page and Parser
Refactor
Site.js
,Page.js
andparser.js
withclass
keyword.Extract out utility functions into separate files for better
consistency.
Re-structure some part of the class for better data encapsulation.