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

fix: tailwind supports routes and islands provided by plugins #2266

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

deer
Copy link
Contributor

@deer deer commented Jan 19, 2024

closes #2159

@mcgear, please take a look at this. I got my comment cleaned up a bit and it's working for my three test cases.

  1. The first is included in this PR, where I've hacked a plugin into an existing tailwind test. I added a components folder with a component, modified the tailwind config to not scan this folder, and then used this component within a route from the plugin. Without my changes, the text is unstyled; with my changes, it works as expected.
  2. The second is outside of this code base (and hasn't left my computer yet), but it's a "real" usage. Previously I was working around the issue by providing a safelist, which I've now removed. Everything is still styled correctly! 🎉 This project follows fresh's pattern of having source and then tests/fixtures, so everything was local.
  3. Finally I wanted to test a remote plugin. What to do when the remote plugin hasn't been updated to use this branch?
export default defineConfig({
  plugins: [{...blogPlugin(blogOptions), location: "https://deno.land/x/fresh_blog@0.0.6/src/plugin/blog.ts", projectLocation: "https://deno.land/x/fresh_blog@0.0.6"}, ga4Plugin(), tailwind()],
});

After hacking the two new properties into the plugin returned by blogPlugin, updating fresh to be my branch (i.e. "$fresh/": "https://raw.githubusercontent.com/deer/fresh/2159_plugin_routes_tailwind/",), and removing the safelist, this is also working.

If you use this branch in your project, I'm curious to see if it works.

I haven't tested this with builds yet, and I know this is already going to fail in CI for some web assembly reason (at least on my computer). But I wanted to put this up since it's solving a pretty big problem with plugins.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL at my last suggestion regarding your DM query. deno_std can be used, but it doesn't make a huge difference. Nor does this code make me think that there's a need for a new API in deno_std.

I'm not really aware of the context behind this PR, so take my suggestions with a pinch of salt, but the code looks good to me 👍🏾

plugins/tailwind/compiler.ts Outdated Show resolved Hide resolved
plugins/tailwind/compiler.ts Outdated Show resolved Hide resolved
plugins/tailwind/compiler.ts Outdated Show resolved Hide resolved
@deer
Copy link
Contributor Author

deer commented Jan 22, 2024

PTAL at my last suggestion regarding your DM query. deno_std can be used, but it doesn't make a huge difference. Nor does this code make me think that there's a need for a new API in deno_std.

I'm not really aware of the context behind this PR, so take my suggestions with a pinch of salt, but the code looks good to me 👍🏾

Thanks for the suggestions -- these look great. I'll incorporate these now and get this cleaned up some more.

@deer
Copy link
Contributor Author

deer commented Jan 22, 2024

todo:

  • fix tests on windows
  • write proper tests for this change

@deer
Copy link
Contributor Author

deer commented Jan 24, 2024

@iuioiua, as we discussed yesterday, I'm working the pluginification of saaskit using this PR. It's going well, except for your usage of "@/": "./" in the import map. This is breaking my module resolution. I logged denoland/deno_graph#364 since it doesn't seem like something I should be doing.

Given that I want to get this done sooner rather than later, I'm trying to do the module resolution myself. Unfortunately I'm encountering denoland/vscode_deno#708 when trying to debug the project. So using console.log to fix the problem is slow going.

@iuioiua
Copy link
Contributor

iuioiua commented Jan 24, 2024

Perhaps, try get this working for local plugins, where all the code for that plugin lies within that plugin's directory. Then, you might be able to just walk() the .tsx files and call Deno.readTextFile(). That seems like an alright v0 of the implementation and I'm sure there'll be a learning opportunity on the way to get you to the next version.

plugins/tailwind/compiler.ts Outdated Show resolved Hide resolved
@deer deer marked this pull request as ready for review January 25, 2024 17:21
@deer
Copy link
Contributor Author

deer commented Jan 25, 2024

@marvinhagemeister @iuioiua, I think this is ready to look at. There's an uptake of this PR in denoland/saaskit#660.

I've added a few different test cases to make sure that the different ways of adding routes via plugins are covered. (Most of the emphasis is on the module resolution code.)

plugins/tailwind/compiler.ts Outdated Show resolved Hide resolved
plugins/tailwind/compiler.ts Outdated Show resolved Hide resolved
.vscode/import_map.json Show resolved Hide resolved
@deer deer mentioned this pull request Jan 29, 2024
@deer deer requested a review from iuioiua January 29, 2024 17:25
@deer
Copy link
Contributor Author

deer commented Jan 30, 2024

I'm not sure whether this should be merged until denoland/vscode_deno#708 is resolved. Otherwise I don't think people will be able to debug their apps.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM

plugins/tailwind/compiler.ts Show resolved Hide resolved
plugins/tailwind/compiler.ts Outdated Show resolved Hide resolved
@deer deer requested a review from iuioiua January 31, 2024 10:13
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. Few things that I didn't realise earlier. Definitely looking better, though.

src/server/types.ts Outdated Show resolved Hide resolved
src/server/types.ts Show resolved Hide resolved
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.

Support tailwind for routes and islands from plugins
2 participants