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

bug: unable to use local pathed modules in config file. #45

Closed
jaymanmdev opened this issue Dec 1, 2023 · 18 comments
Closed

bug: unable to use local pathed modules in config file. #45

jaymanmdev opened this issue Dec 1, 2023 · 18 comments
Assignees

Comments

@jaymanmdev
Copy link

jaymanmdev commented Dec 1, 2023

Hey there! I am ABSOLUTELY LOVING hwy! This framework is what I have been waiting for! I can't wait to see where it goes in the future!

I have encountered a little bit of an issue. I am looking to validate my environment variables in the configuration file so that if they are not valid then I don't proceed with the config start-up. However, I get the following error when trying to use a module within the configuration file:
image

If I pull in the source from the module and use it, it all works fine - npm modules work fine, it is just local modules that don't. It seems like it is resolving it from the dist directory, using the built configuration file. This behaviour bug really only affects development as in production you won't need the hmr browser watcher etc. As the port option in the config is used as the port for the watcher, I currently cannot validate my env, pass it to the config port option and have hmr/reload working properly. I get the following error if I try to reload this way (saving a file in dev, triggering the watcher):
image

As you can see, the watcher is trying to connect/make a request to port 3000 (the default) even though the server is running through port 8000 based on my env variables. I can think of two possible fixes we can make to alleviate this at the moment.

  1. In development, read the hwy.config.ts file in the root of the project and resolve modules properly, rather than reading the built config file in dist.
  2. Run the watcher startup and config inside main.tsx instead, maybe something like this:
const app = new Hono();

await hwyInit({
    app,
    importMetaUrl: import.meta.url,
    serveStatic,
    port: process.env.PORT
});

^^^ Pass the port into hwyInit and start the watcher at that point since this is processed before the server starts anyways. I understand that the main.tsx file is reloaded with the watcher, so maybe this isn't possible without some major reorganization of the server entrypoint?

Thanks heaps!

@sjc5
Copy link
Owner

sjc5 commented Dec 1, 2023

Hey there @jaymanmdev!

Thanks so much for the kind words and super detailed bug report, it's a huge help.

This is a great catch and a good use case, and luckily an easy fix.

Can you give 0.7.2-beta.9 a try (update all your Hwy packages just to be safe), and let me know if it fixes this issue for you?

If so, I can release this isolated fix on short order.

Thanks!

@sjc5 sjc5 self-assigned this Dec 1, 2023
@jaymanmdev
Copy link
Author

jaymanmdev commented Dec 1, 2023

Hey there! Thankyou for being such a responsive and kind/well-mannered maintainer!

I tried to find the source of the issue and a fix for it so that I could contribute but I am still finding my way around the codebase.

I upgraded all of my packages to 0.7.2-beta.9, as well as hono to 3.10.4 and I am getting the following JSX related error when trying to make a request (seems to be Hono related):
image

However, I downgraded back to Hono 3.10.3 which is what I was using last night and it displays the same error, which seemed to work fine last night.
What exactly is the fix that you have made?

Thanks!

@jaymanmdev
Copy link
Author

jaymanmdev commented Dec 1, 2023

@sjc5 I upgraded to 0.7.2-beta.11 and the JSX issue doesn't exist anymore. Still getting the port issue for the watcher though. :)

EDIT: I can confirm that the issue is fixed (I had it set up incorrectly.). Thankyou so much! Woohoo!

@sjc5
Copy link
Owner

sjc5 commented Dec 1, 2023

@jaymanmdev

Re: the new error:

Ah, I think I know what that may be (sorry, I pushed this attempted fix quickly and it's mixed in with some other prior experimentation).

Can you:

  1. Update all Hwy packages now to 0.7.2-beta.11 (.11, not .10).
  2. Update all Hono packages back to latest (including @hono/node-server, if you're using it).
  3. Try again.
  4. If it doesn't work, try nuking node_modules and your lock file, and reinstall all deps.
  5. Try yet again (this has fixed similar errors for me I've seen across recent Hono patch update cache mismatches).
  6. Let me know your results, as well as what runtime you're using.

Re: the fix for the original issue:

Before we were just checking to see if the config file were a JS or TS file. If JS, we just read it raw, and if TS, we ran it through a esbuild.transform (without bundling). In both cases, it's not going to know what to do when trying to import first-party modules, because it's running from dist, as you correctly pointed out. Further, depending on whether your runtime is automatically parsing TS, it may or may not know how to read your source files anyway.

So to support this use case (which is great), instead we can just use esbuild.build (with bundling), read the build output to init the config object, and then delete the build output from dist as it's no longer needed at that point.

@jaymanmdev
Copy link
Author

jaymanmdev commented Dec 1, 2023

@jaymanmdev

Re: the new error:

Ah, I think I know what that may be (sorry, I pushed this attempted fix quickly and it's mixed in with some other prior experimentation).

Can you:

  1. Update all Hwy packages now to 0.7.2-beta.11 (.11, not .10).
  2. Update all Hono packages back to latest (including @hono/node-server, if you're using it).
  3. Try again.
  4. If it doesn't work, try nuking node_modules and your lock file, and reinstall all deps.
  5. Try yet again (this has fixed similar errors for me I've seen across recent Hono patch update cache mismatches).
  6. Let me know your results, as well as what runtime you're using.

Re: the fix for the original issue:

Before we were just checking to see if the config file were a JS or TS file. If JS, we just read it raw, and if TS, we ran it through a esbuild.transform (without bundling). In both cases, it's not going to know what to do when trying to import first-party modules, because it's running from dist, as you correctly pointed out. Further, depending on whether your runtime is automatically parsing TS, it may or may not know how to read your source files anyway.

So to support this use case (which is great), instead we can just use esbuild.build (with bundling), read the build output to init the config object, and then delete the build output from dist as it's no longer needed at that point.

I can confirm that the issue has been fixed and I can use modules inside the config file now. No issues here anymore. I am using node by the way. The deno runtime didn't work for me on a fresh project yesterday so that might be worth looking into (I'm gonna stick with node anyways though).

However, I do wonder if changing the architecture to initiate the watcher in the main.tsx file alongside initHwy would be better, and passing the port into that function, and maybe it can return a custom serve function that can be called directly when needed, rather than having to use the port in the config file, as well as the serve function in main.tsx, if that makes sense. I'm just thinking about the API here. :)

Cheers!

@sjc5
Copy link
Owner

sjc5 commented Dec 2, 2023

@jaymanmdev

Nice! Thanks for confirming.

Also thanks for the tip on Deno, I'll take a look when I get some time. The Deno setup is definitely weirder and more brittle.

Can you clarify if there's a use case or convenience reason you'd rather set the dev server port in your main server entry? Usually that is something that is set in a background .env file (which you can also do and will override the Hwy config btw). IMO ideally your actual source code isn't really aware of it or in charge of setting it.

For a little background, one reason that it is difficult to put too much of the dev server code into your main server entry is that a lot of the watcher-related stuff requires Node APIs, and it can cause errors when bundling for non-Node targets.

It also can get a little trickier architecturally if your watchers are initiated in your main entry file. You sort of lose control over the lifecycle needed to make browser refresh work, because if the main entry file gets restarted, you lose any reference you had to which browser tabs you had open for server-sent events (or you'd need to write them to a file, which is tricky and also not possible for some non-Node targets).

As far as the server instance itself, that is kind of runtime dependent too. Some of them will "just work" if you export default app and others need some more setup. Ideally I want to keep it so that if you want to do something with your server, you can always just go to the Hono docs and look at their examples for the various runtimes, and it will work with Hwy without any extra indirection.

All of that said, if there's a specific use case that isn't being covered now, or the current architecture is blocking anything, I'd love to find a way to at minimum provide an escape hatch.

Let me know what you think!

@jaymanmdev
Copy link
Author

@sjc5

I'll have more of a think about all of this! and update you if I think of anything. I wouldn't mind contributing some code at some point and see if I can get involved with this project as I am getting pretty invested in Hwy now! - I do agree that the way it is designed right now is probably the best way forward as a platform agnostic, and more flexible approach. I've now set up my environment variable validation logic in a pretty neat way once you fixed the issue I was having and things are playing together VERY nicely so that's good.

I'm currently in the early process of working on a Pokemon Roundest by Theo (Ping.gg) clone (https://faster-round.t3.gg) using Hwy. He used NextJS initially and then Astro for his version so I'm gonna be using Hwy entirely, so Hono, HTMX, Node, probably Turso and SQLite for data storage and probably Fly.io or something similar as a deployment target. It'll be fun to get this up and running!

@sjc5
Copy link
Owner

sjc5 commented Dec 2, 2023

@sjc5

I'll have more of a think about all of this! and update you if I think of anything. I wouldn't mind contributing some code at some point and see if I can get involved with this project as I am getting pretty invested in Hwy now! - I do agree that the way it is designed right now is probably the best way forward as a platform agnostic, and more flexible approach. I've now set up my environment variable validation logic in a pretty neat way once you fixed the issue I was having and things are playing together VERY nicely so that's good.

I'm currently in the early process of working on a Pokemon Roundest by Theo (Ping.gg) clone (https://faster-round.t3.gg) using Hwy. He used NextJS initially and then Astro for his version so I'm gonna be using Hwy entirely, so Hono, HTMX, Node, probably Turso and SQLite for data storage and probably Fly.io or something similar as a deployment target. It'll be fun to get this up and running!

Sounds awesome, and yes, it would be great to have some more folks familiar with the code base and able to help out! Let me know if you hit any other issues with the Pokemon clone. Unfortunately our docs are a little bit out of date as well, so make sure you read through the last several release notes to fill in any gaps. I plan to make the docs a lot simpler / faster to update soon, and of course get them up to date with all the latest APIs. That might be a good place to start diving into contributing, perhaps.

Looking forward to checking out your clone once it's done, sounds like a cool stack.

@jaymanmdev
Copy link
Author

jaymanmdev commented Dec 2, 2023

I am coming across an issue at the moment.

I deleted the public/dist and dist directories and then went to run hwy-dev-serve, and am getting the following error:
image

Maybe we aren't checking for these directories properly and writing to them if needed before trying to access them?

I am also getting an undici fetch failed error on a large majority of my fetches in Node 20 or 21, but it works perfectly fine in 19. I lost the stacktrace for it though. It might be node related, I don't quite know.

@sjc5
Copy link
Owner

sjc5 commented Dec 2, 2023

I am coming across an issue at the moment.

I deleted the public/dist and dist directories and then went to run hwy-dev-serve, and am getting the following error: image

Maybe we aren't checking for these directories properly and writing to them if needed before trying to access them?

I am also getting an undici fetch failed error on a large majority of my fetches in Node 20 or 21, but it works perfectly fine in 19. I lost the stacktrace for it though. It might be node related, I don't quite know.

@jaymanmdev

public/dist & dist error

Nice catch on the regression when you delete the dist and public/dist folders! Give 0.7.2-beta.12 a try, should fix it.

fetch issue in different node versions

Have any more info on the fetch issue / disparity between Node versions? Is this fetches coming from within your app, or is it the dev server? The browser reloading uses an RPC of sorts, and there are some known issues on certain versions of Node where there is some goofiness with 127.0.0.1 vs. localhost in certain situations, e.g., node-fetch/node-fetch#1624, nodejs/node#47785, nodejs/node#47785, nodejs/node#40702 – any of those seem related, or is this something totally different?

@jaymanmdev
Copy link
Author

jaymanmdev commented Dec 2, 2023

I am coming across an issue at the moment.

I deleted the public/dist and dist directories and then went to run hwy-dev-serve, and am getting the following error: image

Maybe we aren't checking for these directories properly and writing to them if needed before trying to access them?

I am also getting an undici fetch failed error on a large majority of my fetches in Node 20 or 21, but it works perfectly fine in 19. I lost the stacktrace for it though. It might be node related, I don't quite know.

@jaymanmdev

public/dist & dist error

Nice catch on the regression when you delete the dist and public/dist folders! Give 0.7.2-beta.12 a try, should fix it.

fetch issue in different node versions

Have any more info on the fetch issue / disparity between Node versions? Is this fetches coming from within your app, or is it the dev server? The browser reloading uses an RPC of sorts, and there are some known issues on certain versions of Node where there is some goofiness with 127.0.0.1 vs. localhost in certain situations, e.g., node-fetch/node-fetch#1624, nodejs/node#47785, nodejs/node#47785, nodejs/node#40702 – any of those seem related, or is this something totally different?

@sjc5 I am at the movies with my girlfriend at the moment so I'll give it a try later on this afternoon, and let you know if beta 12 fixes the issue. ☺️
And I'll get the stacktrace for the fetch failures in Node 20 and 21 later on too. The fetch issues come from my own fetch to the Pokémon API (only happens on like half of the requests though...), not an internal Hono or Hwy dev server fetch, that's for certain. I am sure it is a Node bug in anything >= 20, and it is probably not the fault of Hono or Hwy. I'll still get the stacktraces to you later on. 👌 It could be the host difference (between localhost and 0.0.0.0) that is the issue too, I'm not sure. That would be interesting to look into. We'll sort it out in a few hours if you are still around! 🤩

@sjc5 sjc5 mentioned this issue Dec 2, 2023
@jaymanmdev
Copy link
Author

@sjc5

Heya! I am back and I have upgraded to 0.8.0-beta.2. The issue with the dist directories is fixed, and the undici fetch issues seem to be mitigated mostly, however, they are still happening from time to time. Here is the stacktrace:
image

@sjc5
Copy link
Owner

sjc5 commented Dec 2, 2023

@sjc5

Heya! I am back and I have upgraded to 0.8.0-beta.2. The issue with the dist directories is fixed, and the undici fetch issues seem to be mitigated mostly, however, they are still happening from time to time. Here is the stacktrace:

image

Do you know what the rate limit is for the API you're hitting? Feels like rate limiting to me, especially seeing the 'Promise.allSettled'

@jaymanmdev
Copy link
Author

jaymanmdev commented Dec 2, 2023

@sjc5
Heya! I am back and I have upgraded to 0.8.0-beta.2. The issue with the dist directories is fixed, and the undici fetch issues seem to be mitigated mostly, however, they are still happening from time to time. Here is the stacktrace:
image

Do you know what the rate limit is for the API you're hitting? Feels like rate limiting to me, especially seeing the 'Promise.allSettled'

It has no rate-limiting:
image

It only seems to be happening every so often right now so I'll continue to monitor it and see how things go.

Also, I have managed to get @kitajs/html's HTMX attribute d.ts file to work in my Hwy project. It's pretty straightforward - changed one line and it worked. Should I make a pull request to add this into the Hwy repository? or should that be up to the user to retrieve from elsewhere. Because Hwy puts a huge emphasis on using HTMX, it might be nice to have those typed attributes out of the box?

@sjc5
Copy link
Owner

sjc5 commented Dec 2, 2023

@sjc5

Heya! I am back and I have upgraded to 0.8.0-beta.2. The issue with the dist directories is fixed, and the undici fetch issues seem to be mitigated mostly, however, they are still happening from time to time. Here is the stacktrace:

image

Do you know what the rate limit is for the API you're hitting? Feels like rate limiting to me, especially seeing the 'Promise.allSettled'

It has no rate-limiting:

image

It only seems to be happening every so often right now so I'll continue to monitor it and see how things go.

It's possible it's the opposite problem and they actually should be rate limiting 😅

@sjc5
Copy link
Owner

sjc5 commented Dec 2, 2023

@sjc5

Heya! I am back and I have upgraded to 0.8.0-beta.2. The issue with the dist directories is fixed, and the undici fetch issues seem to be mitigated mostly, however, they are still happening from time to time. Here is the stacktrace:

image

Do you know what the rate limit is for the API you're hitting? Feels like rate limiting to me, especially seeing the 'Promise.allSettled'

It has no rate-limiting:

image

It only seems to be happening every so often right now so I'll continue to monitor it and see how things go.

Also, I have managed to get @kitajs/html's HTMX attribute d.ts file to work in my Hwy project. It's pretty straightforward - changed one line and it worked. Should I make a pull request to add this into the Hwy repository? or should that be up to the user to retrieve from elsewhere. Because Hwy puts a huge emphasis on using HTMX, it might be nice to have those typed attributes out of the box?

Sorry I'm on mobile and completely missed your last paragraph @jaymanmdev. Yeah would love to see a repo with that done or a PR if it's simple. If it requires a lot of changes let's discuss it in a new issue and maybe start with an example repo.

@jaymanmdev
Copy link
Author

@sjc5

Heya! I am back and I have upgraded to 0.8.0-beta.2. The issue with the dist directories is fixed, and the undici fetch issues seem to be mitigated mostly, however, they are still happening from time to time. Here is the stacktrace:

image

Do you know what the rate limit is for the API you're hitting? Feels like rate limiting to me, especially seeing the 'Promise.allSettled'

It has no rate-limiting:
image
It only seems to be happening every so often right now so I'll continue to monitor it and see how things go.

It's possible it's the opposite problem and they actually should be rate limiting 😅

Yeah, I have a feeling they receive too much traffic - if that is what I have to deal with for now, I'll deal. I'll probably create an internal cache on the server in the future since the API doesn't really change very much.

@sjc5

Heya! I am back and I have upgraded to 0.8.0-beta.2. The issue with the dist directories is fixed, and the undici fetch issues seem to be mitigated mostly, however, they are still happening from time to time. Here is the stacktrace:

image

Do you know what the rate limit is for the API you're hitting? Feels like rate limiting to me, especially seeing the 'Promise.allSettled'

It has no rate-limiting:
image
It only seems to be happening every so often right now so I'll continue to monitor it and see how things go.
Also, I have managed to get @kitajs/html's HTMX attribute d.ts file to work in my Hwy project. It's pretty straightforward - changed one line and it worked. Should I make a pull request to add this into the Hwy repository? or should that be up to the user to retrieve from elsewhere. Because Hwy puts a huge emphasis on using HTMX, it might be nice to have those typed attributes out of the box?

Sorry I'm on mobile and completely missed your last paragraph @jaymanmdev. Yeah would love to see a repo with that done or a PR if it's simple. If it requires a lot of changes let's discuss it in a new issue and maybe start with an example repo.

Alrighty, sounds good! We can create a separate issue for this in a bit - It is very simple right now. It just extends the Hono.HTMLAttributes interface. It is basically https://github.com/kitajs/html/blob/1e6d2efe6e5aae09e432f72f2335ea0dbf9dd528/htmx.d.ts#L5, but lines 5 to 7 are changed slightly to extend Hono.HTMLAttributes. :) For now this is all I have, but I have some ideas for more HTMX-based niceties that I may work on for Hwy in the future!

@jaymanmdev
Copy link
Author

jaymanmdev commented Dec 2, 2023

@sjc5 we can close this issue now. :) I'm going to make my repository public for my Pokemon app now so if you'd like to take some time to skim around it and give me any suggestions, that'd be great. I'm still new to HTMX so I'm not sure about best practices and stuff. I'd love some suggestions. :) (the dev branch is the up to date one.)

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 a pull request may close this issue.

2 participants