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

Enable hot reloading of the functional mixins #146

Merged
merged 2 commits into from
Jun 20, 2022
Merged

Enable hot reloading of the functional mixins #146

merged 2 commits into from
Jun 20, 2022

Conversation

andgra
Copy link
Contributor

@andgra andgra commented Jun 20, 2022

Hello,

In additional to the #145 I added try ... catch around require of mixin file. In case there is a syntax of any other error in a single mixin file, other mixins will be loaded successfully. Should I write tests for this case?

@ai
Copy link
Member

ai commented Jun 20, 2022

No, we do not need tests for errors

@ai
Copy link
Member

ai commented Jun 20, 2022

I fixed CI. Do you plan to send another PR or we are ready for release?

@andgra
Copy link
Contributor Author

andgra commented Jun 20, 2022

Actually, there is an issue with windows OS. While mixinsDir option is sanitized for the glob package (replace \ with /), mixinsFiles are not.
So paths like mixinsFiles: join(__dirname, 'deps', 'f.js') will always fail on windows.
There is an easy way to fix this, but I didn't include it in current PR because its a different issue

Should I create separate PR for this?

@andgra
Copy link
Contributor Author

andgra commented Jun 20, 2022

P.S. also there is an issue with .CSS files on windows when we use mixinsDir (file extension in upper case), but its very rare case and I dont know how to easily fix it. But if your file extensions always in lower case, there is no issue at all.

@ai
Copy link
Member

ai commented Jun 20, 2022

Thanks for the great research on Windows compatibility. I will wait for PRs.

@andgra
Copy link
Contributor Author

andgra commented Jun 20, 2022

Created #147

@andgra
Copy link
Contributor Author

andgra commented Jun 20, 2022

Thank you, that's all what I wanted to improve

@ai ai merged commit 807b588 into postcss:main Jun 20, 2022
@ai
Copy link
Member

ai commented Jun 20, 2022

Thanks. Released in 9.0.3.

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

Successfully merging this pull request may close these issues.

2 participants