-
-
Notifications
You must be signed in to change notification settings - Fork 32
docs(v7): sync docs with css-tree + ESM #154
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
base: master
Are you sure you want to change the base?
Conversation
… outdated options across README and docs
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.
- README’s CommonJS dynamic import example incorrectly uses top-level
awaitin CJS; suggest wrapping in an async function or using.then(). Parser.mdinconsistently documents theparsesignature (parse(css)vsparse(css, options?)).- Some docs still mix CommonJS
requirewith ESM despite the ESM-only requirement; standardize remaining examples in updated files (e.g., Stringify.md). - Overall changes align well with v7 (css-tree parser, walker registration, option removals), but a few inconsistencies remain that can confuse users.
Summary of changes
- Switched CI badges in README to GitHub Actions and updated project description to reflect css-tree parsing and PostCSS-modeled nodes.
- Updated requirements (Node >= 20.19.0, PostCSS >= 8.4.14) and emphasized ESM-only usage with many examples converted to
import. - Clarified walker usage: helpers are not auto-registered; added
registerWalkers(Container)across docs and examples. - Removed/marked legacy
parse()options as forward-compat only; simplifiedExportsandParserdocs accordingly. - Corrected node-specific docs (Comment, Punctuation, Parentheses, Word) and example imports.
- Various consistency and correctness fixes across docs and examples.
…amic import needed)
…ignature, and convert remaining Stringify examples to ESM
55c945f to
887f581
Compare
|
Pushed updates to address the review notes:
Reference: Node v20.19.0 enables |
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.
- Parser docs still show
## parse(css)while other docs mentionparse(css, options?); align the signature and clarify that options are ignored in v7. - docs/README.md uses top‑level
awaitin a CommonJS note; replace with CJS‑safe examples (Promise/async wrapper) or the Node >= 20.19.0require()approach. - Stringify.md retains some CommonJS
requireexamples despite ESM‑only guidance; convert remaining snippets toimportfor consistency. - Overall, the ESM migration and walker registration guidance look good, but a few inconsistencies remain that could mislead users.
Summary of changes
- Replaced CI badges with GitHub Actions in README and updated project description to reflect css-tree parsing and PostCSS-modeled nodes.
- Updated requirements to Node.js >= 20.19.0 and PostCSS >= 8.4.14; emphasized ESM-only usage and added a CommonJS
require()example in README. - Standardized many examples to ESM
importsyntax across docs; adjusted examples to include walker registration viaregisterWalkers(Container). - Simplified and clarified the parser docs: removed legacy options sections, added notes on forward-compatible
options, and documented css-tree node mappings and source mapping behavior. - Cleaned up and clarified several node docs (Comment, Punctuation, Parentheses) and example imports.
- Marked
ParseOptionsas a v7 placeholder and updated Exports to reflect current API. - Removed SCSS/LESS variable detection example and added a note that v7 only detects
--*variables.
…parser header wording
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.
- CommonJS guidance is inconsistent: README and docs/README prefer dynamic
import()while the minimum Node.js version (>= 20.19.0) canrequire()ESM; standardize onrequire()for clarity. - Stringify docs conflict with v7 behavior by claiming whitespace preservation; update to state that spacing is normalized and adjust the example output.
- Parser examples use
walkFuncswithout showing walker registration; addregisterWalkers(Container)to avoid confusing users. - Minor: clarify walker registration in README Benefits to set expectations upfront.
Additional notes (1)
- Readability |
docs/Stringify.md:149-155
The “Preserving Formatting” section claims that original spacing is preserved, but v7 docs elsewhere state that spacing is normalized and not preserved. This contradiction can cause misuse and incorrect expectations fortoString()behavior.
Summary of changes
- Switched README badges to GitHub Actions and updated the project description to reflect css-tree parsing with PostCSS-modeled nodes.
- Updated Requirements to Node.js >= 20.19.0 and PostCSS >= 8.4.14; added an ESM-only note with CommonJS guidance.
- Standardized many code examples to ESM
importsyntax across docs; added explicit walker registration viaregisterWalkers(Container)in multiple docs. - Aligned API docs:
parse(css, options?)withoptionsaccepted for forward-compat in v7 but ignored; pruned legacy options from Exports/Parser and markedParseOptionsas a placeholder. - Clarified node docs (Comment, Punctuation, Parentheses, Word) and fixed/modernized example imports.
- Removed SCSS/LESS variable examples and replaced with a note that
Word.isVariableonly detects--*in v7.
| root.walkFuncs((func) => { | ||
| console.log(`Function: ${func.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.
This example uses root.walkFuncs(...), but walker helpers are no longer auto-registered. Without registration, walkFuncs won’t be available, which may frustrate readers trying to follow the example.
Suggestion
Prepend walker registration to the example:
import { parse, registerWalkers, Container } from 'postcss-values-parser';
registerWalkers(Container);
const root = parse('calc(100% - 20px) url("bg.jpg") center/cover');
root.walkFuncs((func) => {
console.log(`Function: ${func.name}`);
});Reply with "@CharlieHelps yes please" if you'd like me to add a commit updating this example.
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.
@CharlieHelps yes please
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.
Applied. Added walker registration to the example so walkFuncs works in v7.
- Commit: cdeacdd
Updated snippet
import { parse, registerWalkers, Container } from 'postcss-values-parser';
// Walker helpers are not auto-registered in v7
registerWalkers(Container);
const root = parse('calc(100% - 20px) url("bg.jpg") center/cover');
root.walkFuncs((func) => {
console.log(`Function: ${func.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.
- Stringify docs still claim original whitespace is preserved; v7 normalizes spacing. Update the section and example to reflect normalization.
- Parser environment support lists “Node.js (all supported versions)” which conflicts with the new minimum of Node >= 20.19.0 and ESM-only. Align the statement with Requirements.
- Consider adding a brief caveat in README and docs/README about
require()-ing ESM on Node >= 20.19.0 being limited to synchronous ESM graphs (no top-levelawait). - Walker registration, ESM conversion, and
parse(css, options?)consistency otherwise look solid.
Additional notes (3)
-
Compatibility |
docs/Stringify.md:145-145
This section asserts that original spacing is preserved, which conflicts with the v7 behavior you documented elsewhere (“spacing is normalized”). Keeping this claim will mislead users abouttoString()and the default stringifier output. -
Readability |
docs/Parser.md:215-215
The environment support list says “Node.js (all supported versions)”, but the package now requires Node >= 20.19.0 and is ESM-only. This inconsistency can confuse users about runtime support. -
Readability |
docs/README.md:19-24
Same as README: adding a brief caveat aboutrequire()limitations in CJS on Node >= 20.19.0 will reduce user confusion and support questions.
Summary of changes
- Replaced legacy CI badges with GitHub Actions in README and refreshed project description to reflect css-tree parsing with PostCSS-modeled nodes.
- Updated Requirements to Node.js >= 20.19.0 and PostCSS >= 8.4.14; documented ESM-only usage and added CommonJS
require()guidance for Node >= 20.19.0. - Standardized many examples to ESM
import; removed/clarified legacyparse()options and markedParseOptionsas a v7 placeholder. - Documented walker helper registration (
registerWalkers(Container)) across README and multiple docs; removed claims of auto-registration. - Corrected node-specific docs (Comment, Punctuation, Parentheses, Word), refined
Exports, and alignedParser.mdtoparse(css, options?). - Minor copy and consistency edits across docs, plus example fixes (e.g., URL handling, function walking).
Refreshes README and all docs under
docs/to match the v7 rewrite (css-tree parser, pure ESM) and to address discrepancies called out in #152.Changes
importsyntax).registerWalkers(Container)and update Root/Container/Walker docs and examples.parse()options in v7; markParseOptionsas forward-compat only.Word.isVariableonly detects--*in v7.Closes #153.