search

lmammino / tall

Promise-based, No-dependency URL unshortner (expander) module for Node.js
https://lmammino.github.io/tall/
MIT License
72 stars 7 forks source link

Add meta refresh plugin package #43

Closed karlhorky closed 2 years ago

karlhorky commented 2 years ago

Add meta refresh plugin package to follow redirects in <meta http-equiv="refresh"> tags

Closes #42

karlhorky commented 2 years ago

@lmammino ah cool, I see that #44 was merged!

Would you be open to changing this PR into a plugin as well? I'm not sure when I would get around to this...

lmammino commented 2 years ago

@lmammino ah cool, I see that #44 was merged!

Would you be open to changing this PR into a plugin as well? I'm not sure when I would get around to this...

I am not sure when I'll have time to do this as well unfortunately. What i have in mind is to convert the repo to a monorepo that can host multiple packages (tall core and various plugins).

Meanwhile i published the changes as v5 and added an example in the docs inspired by your use case. Let me know if that's useful for a provisory solution

karlhorky commented 2 years ago

Got it, ok understood. I'll see if I have any time until the end of the year.

Eventually I think this following of the meta http-equiv parsing thing should be standard / core in tall (with the ability to turn it off), so that there are less surprises for users (it "just works" for most types of common redirects)

karlhorky commented 2 years ago

Eventually I think this following of the meta http-equiv parsing thing should be standard / core in tall (with the ability to turn it off), so that there are less surprises for users (it "just works" for most types of common redirects)

@lmammino just looking at this again now, would you be open to having this live in this https://github.com/lmammino/tall repository?

eg. either:

  1. As a part of the tall package
  2. Switching this repo to a monorepo (Yarn Workspaces? Nx/Rush/Turborepo?) and having a new tall-plugin-meta-refresh package in here
lmammino commented 2 years ago

Yes, i'd be interested in having the plugin as part of this repo and I think it's best to convert this to a monorepo and publish the plugin as a separate package (mostly because i expect that plugin to be dependency heavy)

karlhorky commented 2 years ago

Ok, then if you would do the change to turn the repo into a monorepo, I'm open to doing the implementation of the plugin (including researching for minimal libraries which can do this or to look into building a mini-parser, as noted above)

lmammino commented 2 years ago

Awesome. Right now (based on some quick research) I have the feeling that going with htmlparser2 would be the best compromise in terms of simplicity and performance. Feel more than welcome to disagree if you have seen other interesting solutions :)

karlhorky commented 2 years ago

Yeah, I've landed on htmlparser2 before in another project actually - it's worked out pretty well so far. Probably it is the best compromise.

karlhorky commented 2 years ago

Ok so I may have some time to work on this in the coming days, @lmammino would you have time to move this to a monorepo before I get started with that?

karlhorky commented 2 years ago

@lmammino or if you think that you have time to get the monorepo conversion done later, just let me know.

lmammino commented 2 years ago

@lmammino or if you think that you have time to get the monorepo conversion done later, just let me know.

Sorry for the late reply, started looking into this: https://github.com/lmammino/tall/pull/49

lmammino commented 2 years ago

@lmammino or if you think that you have time to get the monorepo conversion done later, just let me know.

Done now, let me know if the new structure makes sense to you.

What I am thinking is that we can create a new folder there called http-equiv or tall-http-equiv-plugin and put an entire subproject there...

karlhorky commented 2 years ago

Cool, thanks for this!

Do you think that it makes sense to follow the convention of adding all the packages in a packages directory in the root? Eg. along with a ./packages/* wildcard in the workspaces configuration

lmammino commented 2 years ago

Cool, thanks for this!

Do you think that it makes sense to follow the convention of adding all the packages in a packages directory in the root? Eg. along with a ./packages/* wildcard in the workspaces configuration

We could technically organise things as we want.

We could keep the current structure and create a subfolder for all the plugins:

We would just need to update the main package.json with:

{
  "workspaces": ["core", "plugins/http-equiv"]
}

What do you think? If you have strong feelings and prefer to go with the standard approach, I am ok with that too... it shouldn't take too long to do a refactoring...

karlhorky commented 2 years ago

Not sure I would describe them as strong feelings towards using the conventional packages/ folder, but definitely feelings around:

  1. Not creating a new standard unless there's a really good reason to do so (and then maybe even publishing some kind of article about why others should also consider using this approach too)
  2. It's nice to be able to quickly navigate a new unfamiliar repository, based on this convention: if there's a packages/ directory, then it can quickly be assumed to be a monorepo without having to look in configuration files. I've seen this pattern a lot and I think it's helpful for the community.

If you want a distinction of whether a package is a plugin, this seems reasonable - the name of the package directory (the directory contained within packages) could include the prefix tall-plugin-, so that alphabetical ordering will group all plugins together.

karlhorky commented 2 years ago

@lmammino thoughts on my comment above?

lmammino commented 2 years ago

Happy to go with a more standard approach. I won't be able to do these changes myself in the next week though. Would you be able to submit a PR?

karlhorky commented 2 years ago

I took a first shot at moving the files here: https://github.com/lmammino/tall/pull/51

I named the new folder in packages to be tall because usually the name of the folder closely matches the package name

Maybe you can approve the GitHub Actions workflow runs and we can see whether I messed anything up

karlhorky commented 2 years ago

@lmammino what do you think of #51?

lmammino commented 2 years ago

@lmammino what do you think of #51?

Great work! Just merged it! Thank you so much

karlhorky commented 2 years ago

Nice, great, thanks for the merge! I'll rebase and start implementing the plugin with htmlparser2.

karlhorky commented 2 years ago

Ok I opened some new PRs:

And then, based on that, I created 2f222198e1043ea271cd4edb371a2ce3038e97be with the first version of the plugin using htmlparser2.

Is this what you had in mind?

karlhorky commented 2 years ago

@lmammino is this how you imagined the dependency tall would be specified as a dependency + imported? (I have it as under peerDependencies and devDependencies in the plugin.

The CI is having trouble locating the module when running the build though...

Run npm install
npm ERR! code 1
npm ERR! path /home/runner/work/tall/tall/packages/tall-plugin-meta-refresh
npm ERR! command failed
npm ERR! command sh -c -- npm run build
npm ERR! > tall-plugin-meta-refresh@1.0.0 build
npm ERR! > tsc -p tsconfig.json && tsc -p tsconfig-cjs.json && echo '{"type":"commonjs"}' > lib/cjs/package.json
npm ERR! 
Error: npm ERR! src/index.ts(3,30): error TS230[7](https://github.com/lmammino/tall/actions/runs/3237875566/jobs/5305448461#step:6:8): Cannot find module 'tall' or its corresponding type declarations.
npm ERR! npm ERR! Lifecycle script `build` failed with error: 
npm ERR! npm ERR! Error: command failed 
npm ERR! npm ERR!   in workspace: tall-plugin-meta-refresh@1.0.0 
npm ERR! npm ERR!   at location: /home/runner/work/tall/tall/packages/tall-plugin-meta-refresh

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2022-[10](https://github.com/lmammino/tall/actions/runs/3237875566/jobs/5305448461#step:6:11)-[12](https://github.com/lmammino/tall/actions/runs/3237875566/jobs/5305448461#step:6:13)T21_02_22_577Z-debug-0.log
Error: Process completed with exit code 1.
karlhorky commented 2 years ago

If you can fix the dependency + import, then I think aside from that, this PR is good to go.

codecov[bot] commented 2 years ago

Codecov Report

Merging #43 (59b2575) into main (051fe05) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 59b2575 differs from pull request most recent head 1c4a965. Consider uploading reports for the commit 1c4a965 to get more accurate results

@@            Coverage Diff            @@
##              main       #43   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         2    +1     
  Lines           35        53   +18     
  Branches         5        10    +5     
=========================================
+ Hits            35        53   +18
Impacted Files Coverage Ξ”
packages/tall-plugin-meta-refresh/src/index.ts 100.00% <100.00%> (ΓΈ)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

karlhorky commented 2 years ago

πŸš€πŸš€