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

Add meta refresh plugin package #43

Merged
merged 41 commits into from
Oct 15, 2022
Merged

Add meta refresh plugin package #43

merged 41 commits into from
Oct 15, 2022

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Feb 20, 2022

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

Closes #42

src/index.ts Outdated Show resolved Hide resolved
@karlhorky
Copy link
Contributor Author

@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
Copy link
Owner

lmammino commented Mar 11, 2022

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

karlhorky commented Aug 24, 2022

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

@lmammino
Copy link
Owner

@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: #49

@lmammino
Copy link
Owner

@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
Copy link
Contributor Author

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
Copy link
Owner

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:

  • core (main tall package)
  • plugins
    • http-equiv (tall-http-equiv-plugin package)
    • ... (moar 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
Copy link
Contributor Author

karlhorky commented Aug 26, 2022

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
Copy link
Contributor Author

@lmammino thoughts on my comment above?

@lmammino
Copy link
Owner

lmammino commented Sep 7, 2022

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
Copy link
Contributor Author

karlhorky commented Sep 8, 2022

I took a first shot at moving the files here: #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 karlhorky mentioned this pull request Sep 8, 2022
@karlhorky
Copy link
Contributor Author

@lmammino what do you think of #51?

@lmammino
Copy link
Owner

@lmammino what do you think of #51?

Great work! Just merged it! Thank you so much

@karlhorky
Copy link
Contributor Author

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

@karlhorky
Copy link
Contributor Author

Ok I opened some new PRs:

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

Is this what you had in mind?

@karlhorky karlhorky changed the title Add first naïve meta http-equiv parsing Add meta refresh plugin package Sep 15, 2022
@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

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

❗ 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.

Comment on lines -25 to -26
restore-keys: |
nodeModules-
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed these to avoid restoring the node_modules cache for other versions of Node.js - this can sometimes lead to weird build errors when native modules need to be built differently for different versions of Node.js

package.json Show resolved Hide resolved
Co-authored-by: Karl Horky <karl.horky@gmail.com>
@karlhorky
Copy link
Contributor Author

🚀🚀

@lmammino lmammino merged commit 6d821e0 into lmammino:main Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support following <meta http-equiv="refresh" /> ?
2 participants