seriouslysean / monster-hunter-now-events

A tool that auto-generates calendar events for Monster Hunter Now by scraping web news articles, processing them with AI, and creating a convenient calendar subscription.
https://seriouslysean.github.io/monster-hunter-now-events/
MIT License
9 stars 9 forks source link

Feature/typescript #36

Closed chris--jones closed 11 months ago

chris--jones commented 11 months ago

Issue

Testing Steps

  1. Run the scripts as normal! - I've updated all the package.json scripts to use ts-node for convenience
  2. Run tsc (npm run tsc) to compile the typescript into regular javascript - this will output into the dist folder maintaining the folder structure

Additional Information

  1. I tried to maintain the same eslint rules by utilising airbnb-typescript/base, I had to add an additional rule for console.log as the new parser/plugins didn't like it; I also added { endOfLine: 'auto' } for prettier since it didn't seem to like my line-endings (I'm on windows), lint:fix still seems to replace them with line-feeds though.
  2. The file imports still need to reference .js files even though the extensions are .ts. This is a quirk of the way typescript works and I went down a rabbit hole trying to make sense of it. I think you can use extensionless imports with some additional config or plugins, but it seemed more trouble than it was worth!
  3. Although Typescript supports import.meta, it doesn't fill in the npm_package_version so it's using process.env, again I think this might be solvable, but not without some extra work.
seriouslysean commented 11 months ago

This is straight up wild, thanks! Using the dist folder seems fine with me, but you're going to need to test it with the output of those generation scripts. I have them using the dist folder for outputting the saved events and I feel like it would change the logic with how that flow works.

chris--jones commented 11 months ago

This is straight up wild, thanks! Using the dist folder seems fine with me, but you're going to need to test it with the output of those generation scripts. I have them using the dist folder for outputting the saved events and I feel like it would change the logic with how that flow works.

I considered the shared directory, but couldn't think of a more appropriate output name since it's where all generated content is going and the src/dist directories is a common pattern.

I have tested it though! The dist folder is not cleared (files are simply updated/overwritten) and there is no conflict as far as I can tell. image

Everyone knows that naming things is the hardest, but I could change the tsconfig to go to "build" or change the generation scripts to go to another directory - "generated"?

As an aside I don't pay the premium for gpt-4, so I tested this with gpt-3.5 and it still seemed to work well; perhaps this could be an env option?

seriouslysean commented 11 months ago

This is awesome, thank you! Want to add 3.5 as an option? I think it's a great idea. I really appreciate the work.

Hit me up on discord sometime if you'd like to chat at nilbits.

seriouslysean commented 11 months ago

@chris--jones just merge main to update your branch and this is good to go 👍

chris--jones commented 11 months ago

This is awesome, thank you! Want to add 3.5 as an option? I think it's a great idea. I really appreciate the work.

Hit me up on discord sometime if you'd like to chat at nilbits.

I'll have a think about it - I was just thinking of adding another .env var, but that would allow any arbitrary model to be defined which doesn't seem robust after just adding type checking. Perhaps a nice balance would be GPT_VERSION with a numeric value; that could be a bit future-proof too assuming the API contract doesn't change.

@chris--jones just merge main to update your branch and this is good to go 👍

It looks like you've already merged main in, but the lint status action is now failing because of missing eslint 😦 The dependency installation was skipped due to cache, so there might be something amiss there.

seriouslysean commented 11 months ago

Fun, I'll merge anyway and figure it out later. May need to remove the cache and/or bust by another metric.

Away mostly with phone access so managing has been... interesting, haha.