-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: Run zmd client side #268
feat: Run zmd client side #268
Conversation
packages/zmarkdown/client/script.js
Outdated
minSize: 200 | ||
}) | ||
|
||
// console.log("zmdParser: ", zmdParser); |
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.
Irregular whitespace not allowed no-irregular-whitespace
packages/zmarkdown/client/script.js
Outdated
@@ -0,0 +1,6 @@ | |||
Split(['#left', '#right'], { | |||
sizes: [50, 50], | |||
minSize: 200 |
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.
Missing trailing comma comma-dangle
packages/zmarkdown/client/script.js
Outdated
@@ -0,0 +1,6 @@ | |||
Split(['#left', '#right'], { |
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.
'Split' is not defined no-undef
@Heziode Any progress here? Some guidance:
|
No progress, I do not have a lot of time. |
Instead of remove image download and latex transpiler, can we use an "plugin" architecture like Vue.js, remark and Cie ? Like that we can allow user (developer) to use what it want. |
Yes, all we currently have is plugins. You still need to remove image downloads because browsers don't have access to the filesystem. |
Ok, I will do that. |
client html can load plugins. Example plugin can be found in `plugins/client/zhtml.js`
packages/zmarkdown/zmdParser.js
Outdated
}) | ||
|
||
// get a unique list of languages used in input | ||
const languages = new Set() |
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.
'Set' is not defined no-undef
const mdProcessor = getHTMLProcessor() | ||
|
||
if (typeof cb !== 'function') { | ||
return new Promise((resolve, reject) => |
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.
'Promise' is not defined no-undef
@vhf Can you "pre-review" the code (for the structure) and tell me if you agree with this ? |
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.
Definitely a good start 👍
Could you please ignore the path packages/zmarkdown/dist/
? No need to have these at this point
packages/zmarkdown/index.js
Outdated
@@ -49,6 +28,8 @@ const rebberStringify = require('rebber/src') | |||
const remarkConfig = require('./config/remark') | |||
const rebberConfig = require('./config/rebber') | |||
|
|||
const zParser = require('./zmdParser') |
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.
Could you please use kebab-case for filenames (here parser
would be fine) and name the variable parser
as well? (see other comment about this file)
|
||
const rehypeStringify = require('rehype-stringify') | ||
|
||
const jsFiddleAndInaFilter = node => { |
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.
Most of this file seems duplicated from the other one. What would you think of factoring the common parts out in a file that would define all of these utilities and exports them separately? (This file would also replace zmdParser.js
)
// …requires
// …definitions
module.exports = {
foo,
bar,
baz
}
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.
that's what I meant by "clean code", refactor code.
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.
To me this would be a structural change, not a minor cleanup. Would you do it that way or what are your thoughts?
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 have a little idea of how to do that. I will do that tomorrow.
packages/zmarkdown/.babelrc
Outdated
@@ -0,0 +1,9 @@ | |||
{ | |||
"presets": ["env"], |
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.
Please add targets
in here
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.
How do you want to configure that ? I do not have any idea of how to configure that.
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'd say > 0.5%, last 2 versions, Firefox ESR, not dead, ie >=11
packages/zmarkdown/client/index.js
Outdated
} | ||
|
||
// Check the object structure | ||
if (!('name' in obj)) { |
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 use in
to check for the presence of a key, prefer hasOwnProperty
(same for the rest of the file)
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.
Can you please explain why use hasOwnProperty
instead of in
?
Ok, I have my answer with The Uses of 'in' vs 'hasOwnProperty'.
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's more precise and corresponds better to what we want to achieve here. We don't care about a method obj.name()
(i.e. YourObjectsPrototype.prototype.name
) for instance, what we're looking for is a property.
packages/zmarkdown/client/index.js
Outdated
// Check the object structure | ||
if (!('name' in obj)) { | ||
throw new Error("missing 'name' in plugin") | ||
} else if (typeof obj.name !== 'string') { |
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.
else
is useless here (same for the rest of the file)
In `render` function, add possibility to use callback in the second parameter by omitting 'type' parameter. In 'use', we check if the parameter is an object.
packages/zmarkdown/client/index.js
Outdated
if (!cb) { | ||
cb = type | ||
} else if (typeof cb === 'function') { | ||
throw new Error("Non deterministic use of cb due to type of 'type' and type of " + |
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.
Extra args are always allowed in JavaScript, we should silently ignore this case.
const mdProcessor = getHTMLProcessor(remarkConfig, extraPlugins) | ||
|
||
if (typeof cb !== 'function') { | ||
return new Promise((resolve, reject) => |
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.
'Promise' is not defined no-undef
@vhf, The feature is globally complete, but, I have some trouble with tests. I do not understand why it not pass. And now the "inspect" break the object. |
Nice! Take a look at the logs: https://travis-ci.org/zestedesavoir/zmarkdown/jobs/424714133 , I'm sure you'll find out why tests are failing. @Heziode I'm pretty sure that by reading the diff while doing this review I found the bug you're looking for. See comments below |
.babelrc
Outdated
@@ -5,5 +5,6 @@ | |||
"development": { | |||
"sourceMaps": "inline" | |||
} | |||
} | |||
}, | |||
"targets": "> 0.5%, last 2 versions, Firefox ESR, not dead, ie >=11" |
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 config should be in https://github.com/zestedesavoir/zmarkdown/pull/268/files#diff-3073df2720eaad6e9a1b54da7f5aab4b
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.
Bad link ? I do not know where you want me to put it.
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.
Ok, my bad, I have understood.
Ignore css files generated by zds-site
Good progress. Could you please remove codacy (we don't use it in this project) and update the README? After that I'll try this PR again locally (building it using the instructions you'll provide), do a last round of review and we'll be good to go. |
It is done. |
Due to portage of remark-iframes from python version. In python the parameter feature was deleted from URL. This cause trouble on the client.
@vhf if you don't use codacy for this project, can you disable it ? It fails the PR. |
oups, my bad. |
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.
Thanks @Heziode , looking really good!
packages/zmarkdown/common.js
Outdated
|
||
function getLatexProcessor (remarkConfig, rebberConfig, target) { | ||
remarkConfig.noTypography = true | ||
if (extraRemarkPlugins && extraRemarkPlugins.length > 0) { |
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 think you could remove this condition if you do const zmdParser = (config, extraRemarkPlugins = []) => {
on l.70.
packages/zmarkdown/common.js
Outdated
if (extraRemarkPlugins && extraRemarkPlugins.length > 0) { | ||
for (const record of extraRemarkPlugins) { | ||
if (!record.check || record.check(config)) { | ||
if (record.option) { |
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.
In the README you wrote that passing null
as option means no configuration for the plugin. Is this really what we want? What if a plugin has a default config and passing null
disables it? In this case we couldn't pass null
.
I think there is some confusion between the semantics of null
and undefined
. A missing config would be undefined
, it would use a default value if any. Explicitly passing null
should still be possible.
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.
Some remark plugin doesn't have config. The null
is for these plugins
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 still think this condition is incorrect. I think you are confusing null
with undefined
.
Imagine the following plugin:
(config) => {
if (typeof config === 'undefined') config = defaultConfig // this is basically (config = defaultConfig) => {}
doSomething(config)
}
How do I pass null
to doSomething
? (if (record.option)
will be false
when record.option === null
, which is why this condition is not correct.)
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.
In other words: the config should be undefined unless you define some config.
Defining the config as null
means that null
is the config, it is defined as null
and therefore it is not undefined.
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.
There are plenty of other cases.
if (config) useConfig(config)
doesn't let you use null
or false
or 0
or ''
as config. All of these values could be used by plugins authors.
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.
Indeed, I have do the change.
packages/zmarkdown/config/remark.js
Outdated
@@ -29,6 +29,7 @@ const remarkConfig = { | |||
blocks: [], | |||
}, | |||
|
|||
canUseTextr: true, |
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'd prefer something like useTextr
or enableTextr
or (with the value as false
) disableTextr
. It's not that this option let's us decide later whether we want to use textr or not, this option explicitly enables it.
packages/zmarkdown/server.js
Outdated
function rendererFactory (config, target) { | ||
if (target === 'html') { | ||
return zmd.rendererFactory(config) | ||
} else if (target === 'latex') { |
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 seems to me that else
is useless here?
I do review change asap. |
Logicaly, on zmarkdown this should be fine as no other member would have set it up. The currrent "failure" was due to the fact I, one day, added zmarkdown in my own codacy dashboard. I removed it, so on next commit my "codacy warning" won't show up. |
packages/zmarkdown/common.js
Outdated
@@ -213,7 +211,7 @@ module.exports = ( | |||
opts = {} | |||
} | |||
|
|||
if (!opts.remarkConfig || (opts.remarkConfig === null) || !Object.keys(remarkConfig).length) { | |||
if ((opts.remarkConfig === null) || !opts.remarkConfig || !Object.keys(remarkConfig).length) { |
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.
See #268 (comment)
packages/zmarkdown/common.js
Outdated
if (extraRemarkPlugins && extraRemarkPlugins.length > 0) { | ||
for (const record of extraRemarkPlugins) { | ||
if (!record.check || record.check(config)) { | ||
if (record.option) { |
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 still think this condition is incorrect. I think you are confusing null
with undefined
.
Imagine the following plugin:
(config) => {
if (typeof config === 'undefined') config = defaultConfig // this is basically (config = defaultConfig) => {}
doSomething(config)
}
How do I pass null
to doSomething
? (if (record.option)
will be false
when record.option === null
, which is why this condition is not correct.)
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.
Could you please add some build instructions to the README?
- How to get the live reload dev build mode?
- How to build a production bundle, minified etc?
packages/zmarkdown/public/index.html
Outdated
<pre class="result render latex-result"> | ||
</pre> | ||
</div> | ||
<script src="../dist/zmarkdown.js"></script> |
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 will be an issue to deploy to github pages because this file will be at the root of the branch.
I suggest to delete the current root public
symlink and creating a symlink from here ./js -> ../dist
(or ./dist
). In the travis deploy script I'll then copy the content before pushing.
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.
Here would be the travis config:
git:
depth: 10
language: node_js
cache:
directories:
- node_modules
node_js:
- '8'
- '10'
env:
- DEST=/tmp
global:
secure: V3Wlrzc7mpyVxNxeBPMkfyowycurM2OmkTTc3aeUkRjrdltRNfknudhC2OELSpsfMvUTLLSPW/oMfv+jxgVHOydVIsBbN+lB0jKnm/T+ABfUeDw+tpEEvKvKLtD/uxDB3rC83uD9ociE82PqdGYtBOJX4Sug5bdoq8n02DlsGd1dnsr1dIdIIX0LHp4OkfRF6jv0EyXwSHK7Kk6s/miPtuChHrhJQwsjJBu5YPdSEu+f0mzilEO5NUiv4hT1IWjfBravP6uTyWIUcwtfY1rgmjZy3d+MaCRKbLi4hrzrIa/VL/rX9fg8vN6Jgnt1+kJtPB7Y0yH2tcl0tpeRWW2ppBZOUU2rJ5OWA8TKltZZW7zjQgrdrwNns0+8ifZ7AI/Jdfvfj0NjW/SqsxchLQL2uN0FsBkoUV6Ly4cwZ/B5EIFTdi0qnjcYBdHoXE21j81clk0rKdgiof/lGOL4+bA59O7V88AEOmgGshRF1AUbe1SwBQ6l8+w44ZDg7I9OQfNSf/W07NbT4cwSF+xp4nc0Rw0EzHnfHLxdYv0Dj+1GxAQdtJ22wmN5JFKlENsfslC6bHT2ZN2V5U0K2VqCBdh0iNdV/uV9tdy3GCSnO3W2XuBKtDbo+wrLf8rjIubWwupej8JxR+pPJ10KNJN5rhe/pqIRujr5FxUZJG7mll9w9Ws=
before_install:
- npm install -g pm2
install:
- npm install
- npm run bootstrap
before_script:
- pm2 ping
after_success:
- cat ./coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js
- rm ./packages/zmarkdown/public/js
- cp ./packages/zmarkdown/dist/* ./packages/zmarkdown/public/js/
deploy:
provider: pages
skip-cleanup: true
github-token: "$GITHUB_TOKEN"
keep-history: false
target-branch: gh-pages
local-dir: ./packages/zmarkdown/public
on:
branch: master
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.
If I well understood what was asked, it's good now.
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.
Thanks for all your hard work on this!
This PR aims at providing a client-side bundle of zmarkdown.
Related to #259
Note: This is currently a Work In Progress
Client Architecture
The architecture of the client is similar to
remark
orVue
. The manager, hereclient/client.js
, exposes the variableZMarkdown
. This manager doesn't work alone because it does not know how to convert the input to the desired output. Example, if you want to convert markdown to html, the manager doesn't know how to do this. It need modules to know how to do this.Manager
You need to add modules with
ZMarkdown.use(obj)
to manage rendering.To convert a markdown string you should use
ZMarkdown.render(str, moduleName = null, cb = null)
:string
, string to convertstring
, name of the module you want to use. This parameter can be omitted only if you define a default module withZMarkdown.setDefaultModule
(you can reset default type withZMarkdown.resetDefaultModule
)function(err, vfile)
, called when process is doneThis function return a
Promise
if no callback specified.ZMarkdown
have also aparse(moduleName)
function to get the MDAST tree andgetParser(moduleName)
to get the whole parser. This parameter can be omitted only if you define a default module withZMarkdown.setDefaultModule
.Module
The module is an object that should have the following properties:
string
, a string that define the name of the plugin. This is used to identify each pluginsfunction(input, cb): void
, a function called inZMarkdown.render
. It returns aPromise
if no callback specifiedfunction(input): object
, get MDAST treefunction(): object
, get the whole of parserfunction(config)
, configure the module with a custom configuration. You do not have to call this function if you want to use the default configurationTips
You can start a module with a base parser using
./common.js
. The module exports a function that can take two optionals parameters:object
, your remark config (defaults to the configuration fromconfig/remark
)array of objects
, remark plugins you want to add to the default parser (remark pipeline). The object should contain:remark plugin
function(config)
optional, a function that returns a boolean to use this plugin depending to remark config passing in argument ofcommon
function(config)
(defaultgetHTMLProcessor
of./common.js
), processor function used to configure the remark pipeline for you outputModule example:
Modules need to be bundled for used on the client.
Here is a webpack conf of the previous example:
What was done
Uncaught (in promise) TypeError: URLSearchParams is not a constructor
Todo