Pass .posthtmlrc options to posthtml-parse #1316
Conversation
luikore
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1316 +/- ##
=========================================
+ Coverage 86.93% 88.64% +1.7%
=========================================
Files 80 80
Lines 4286 4341 +55
=========================================
+ Hits 3726 3848 +122
+ Misses 560 493 -67
Continue to review full report at Codecov.
|
|
@devongovett |
|
|
||
| module.exports = async function(asset) { | ||
| async function parse(code, asset) { | ||
| var config = await getConfig(asset); |
There was a problem hiding this comment.
Is the configuration format identical in parser and transform?
There was a problem hiding this comment.
Yes, but different options are used. Do you prefer these configs to be different objects?
There was a problem hiding this comment.
As long as they can’t conflict it should be fine but it sounds like they can
There was a problem hiding this comment.
How about putting these options into a new htmlparser2rc ?
There was a problem hiding this comment.
I was thinking more of something like this:
.posthtml
{
...posthtmlOptions,
parser: {...parserOptions}
}
There was a problem hiding this comment.
But as you said they won't conflict I think it's safe for now to just let this PR be as is
There was a problem hiding this comment.
I think this will result in getting the config twice... once for parse, and once for transform. Maybe ok because of the cache? Just pointing that out.
src/transforms/posthtml.js
Outdated
| return config | ||
| } | ||
|
|
||
| module.exports = { |
There was a problem hiding this comment.
this can be written as module.exports = {parse, transform};
src/transforms/posthtml.js
Outdated
| {packageKey: 'posthtml'} | ||
| ); | ||
| let config = | ||
| asset.getPackage().posthtml || |
There was a problem hiding this comment.
asset.getPackage() returns a Promise so asset.getPackage().posthtml always evaluates to undefined :
Lines 127 to 133 in 1614918
Use await :
(await asset.getPackage()).posthtml || ...
src/transforms/posthtml.js
Outdated
| if (!config && !asset.options.minify) { | ||
| return; | ||
| } | ||
|
|
||
| config = Object.assign({}, config); | ||
| config.plugins = await loadPlugins(config.plugins, asset.name); | ||
| config.skipParse = true; | ||
| return config; | ||
| return config |
There was a problem hiding this comment.
Looks like Prettier did not format (--no-verify?), you can manually run it by running yarn format or npm run format.
There was a problem hiding this comment.
Did not know this before, updated
src/assets/HTMLAsset.js
Outdated
| constructor(name, options) { | ||
| super(name, options); | ||
| constructor(name, pkg, options) { | ||
| super(name, pkg, options); |
There was a problem hiding this comment.
We don't pass pkg to the constructor anymore for perf reasons :
constructor(name, options) {
super(name, options)
| '.posthtmlrc', | ||
| '.posthtmlrc.js', | ||
| 'posthtml.config.js' | ||
| ])); |
|
|
||
| module.exports = async function(asset) { | ||
| async function parse(code, asset) { | ||
| var config = await getConfig(asset); |
There was a problem hiding this comment.
I think this will result in getting the config twice... once for parse, and once for transform. Maybe ok because of the cache? Just pointing that out.
This pull request makes HTML parsing options configurable, so HTMLAsset parsing / transform behavior can be more consistent.
With this change we can config
.posthtmlrcwith the following options (they are consumed by posthtml-parse, then htmlparser2. See also https://github.com/fb55/htmlparser2/wiki/Parser-options )xmlMode: truelowerCaseAttributeNames: falselowerCaseTags: falserecognizeSelfClosing: trueIf we config
recognizeSelfClosing: truein.posthtmlrc, we can also fix #1103Allow setting
falseto lower case options maybe can also fix #1123 . (not tested yet)