Skip to content
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

Use the dedicated PostCSS API for errors #518

Closed
binyamin opened this issue Jan 18, 2023 · 2 comments · Fixed by #540
Closed

Use the dedicated PostCSS API for errors #518

binyamin opened this issue Jan 18, 2023 · 2 comments · Fixed by #540

Comments

@binyamin
Copy link

PostCSS has a dedicated API method for errors. This codebase is inconsistent. It only uses it in lib/assign-layer-names.js.

For example:

throw new Error(
`Failed to find '${id}'
in [
${paths.join(",\n ")}
]`

Also:

postcss-import/index.js

Lines 261 to 265 in b7d1939

throw new Error(
`Incompatable @charset statements:
${stmt.node.params} specified in ${stmt.node.source.input.file}
${charset.node.params} specified in ${charset.node.source.input.file}`
)

@binyamin
Copy link
Author

binyamin commented Jan 18, 2023

Also, may I rewrite the code using ESM? Or at least using async functions, instead of using Promise? It might be easier to read.

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 19, 2023

Good call on the error API; PR welcome.

As for async: I agree it would be an improvement; I just never got the time to devote to a full codebase refactor. I'm also unsure how good our test coverage is at the moment, we should probably get some coverage measurement in first and make sure that's in tip-top shape before attempting an async refactor. If you want to take it on, more power to you!

I'm unsure if I want to migrate to ESM right away; since ESM modules cannot be imported in CJS. Support for postcss.config.mjs landed less than a year ago, there's probably quite a few people still using it; I don't want to cause unnecessary ecosystem churn. In any case, that would be best done as a separate refactor, after async.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants