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

feat: support bundling of css @import rules #278

Merged
merged 2 commits into from
Aug 18, 2024

Conversation

nobkd
Copy link
Collaborator

@nobkd nobkd commented May 27, 2024

Support bundling of @import rules, useful, e.g. when importing CSS from node_modules

TODO:

  • Write a simple test that involves @import

@tipiirai
Copy link
Contributor

Seems like a legit feature. One thing to note that hot-reloading is not supported for @import:ed files since Nue has no way of tracking those dependencies (without writing a Lightning CSS extension, but that feels like an overkill)

@nobkd nobkd marked this pull request as ready for review May 29, 2024 21:06
@nobkd
Copy link
Collaborator Author

nobkd commented May 29, 2024

The test for Windows seems to fail, because of the code on master.
Here is the same run, but just for the branch (I reset to before the last merge commit, to be able to test without jest issues): https://github.com/nobkd/nue/actions/runs/9293144796

The thing about not being able to trace @imported files for hot reloading should probably be mentioned in the docs then. This also probably only occurs, when the CSS file is in a local project, and not from a package... I agree, that it would be hard to track changes for imported files, and probably isn't needed.
We can still think about the possibility of being able to modify the lightningcss config that is passed to the transformation/bundling (#220)

@nobkd nobkd requested a review from tipiirai May 29, 2024 21:17
@nobkd

This comment was marked as off-topic.

@nobkd nobkd changed the base branch from master to dev June 9, 2024 21:03
@nobkd nobkd changed the base branch from dev to master July 19, 2024 22:57
@nobkd nobkd changed the base branch from master to dev August 11, 2024 16:28
@nobkd nobkd force-pushed the feat/process-import-rules branch from 3d7ad17 to 10c4871 Compare August 11, 2024 17:12
@tipiirai
Copy link
Contributor

@import sounds like the right solution to include Glow styling from node_modules, instead of hacking the dependency inside nuekit

@nobkd nobkd force-pushed the feat/process-import-rules branch from 10c4871 to 4a1e016 Compare August 14, 2024 18:18
@nobkd
Copy link
Collaborator Author

nobkd commented Aug 14, 2024

@import sounds like the right solution to include Glow styling from node_modules, instead of hacking the dependency inside nuekit

I'm not sure, how exactly you mean that 🤷

But just, so you know: Imports from node_modules have to provide the path to/with node_modules (relative or absolute), it is not resolved automatically by lightningcss.

So I don't know how exactly you would do that when glow is just installed (globally) because of nuekit or locally, because nuekit is local, so...

@tipiirai tipiirai merged commit 157c281 into nuejs:dev Aug 18, 2024
4 checks passed
@nobkd nobkd deleted the feat/process-import-rules branch August 18, 2024 04:40
@tipiirai
Copy link
Contributor

So the syntax of importing from node_modules is something like this ?

@import url("node_modules/nuekit/browser/syntax.css")

And the imported CSS is always bundled inside the parent CSS regardless of where it is imported?

The above would allow me to get rid of the automagical syntax.css auto-inclusion:

https://github.com/nuejs/nue/blob/master/packages/nuekit/src/site.js#L279

Thanks!

@nobkd
Copy link
Collaborator Author

nobkd commented Aug 18, 2024

I'm not sure. Probably. It's too long in the past that I did the code changes. Also I not completely sure, what exactly you want to do.

You probably can use the resolvePath from init.js to get an absolute path to the css.
But what's next?


BTW: I forgot to change the merge request branch to point to master. So the changes from here are on dev now, but not on master. (edit: merged dev to master)

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