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

[feat] json feed loader #160

Merged
merged 12 commits into from
Apr 16, 2024
Merged

[feat] json feed loader #160

merged 12 commits into from
Apr 16, 2024

Conversation

upteran
Copy link
Contributor

@upteran upteran commented Apr 12, 2024

Feature #8

  • Add json feed loader and tests
  • Add to createTextResponse[download.ts] json parse handler
  • Add tests with json-feed loader to preview.test

Motivation

  • Inside the json feed loader own implementation of the Loader methods. Reusing something from the rss/atom loaders probably won't work because the jsonfeed has its own structure and content type
  • Update the createTextResponse and add a method to handle JSON text. The implementation within the parse method will be more complicated because it will add a new text media type. Currently, parse returns a document structure, which is why we will need many checks and typings to prevent incorrect type returns.

Questions

  • What order of loaders we should use ? Now it's: atom, jsonfeed, rss

Checklist

  • Don’t rush. Check all changes in PR again.
  • Run pnpm test.
  • Think about changing documentation.
    • If you added a script to scripts/, add a comment with a description.
    • If you added a new folder, add its description to the project’s README.md.
    • If you added config, describe how we use this tool in the config’s comment.
    • If you added something to the project’s architecture, describe it in the project’s README.md.
    • Try to focus on “why?”, not “how?”.
  • If you added a new dependency, check our requirements.
  • Think about testing
    • If you added a feature, add unit tests.
    • If you added a new state to the UI, add visual tests.
    • If you fixed the bug, think about preventing bug regression in the future.
  • If you changed web client:
    • Think about moving code to core/. What code will also be useful on other platforms?
    • Run pnpm size and check the difference in the JS bundle size. Is it relevant to the changes? Change the limit in web/.size-limit.json if necessary.
    • The UI was checked in Chrome and Firefox (and Safari or Epiphany if you have them).
    • Think about keyboard UX. Is it easy to use the new feature with only one hand on a keyboard? Is it easy to understand what keys to press?
    • Think about HTML semantics.
    • Think about accessibility. Check a11y recommendations. Think about how screen reader users will use the tool. Is it easy to use on a screen with bad contrast?
    • Think about translations.
    • Think about right-to-left languages. What parts of the screen should be mirrored for Arabic or Hebrew languages?
  • If you changed the colors token in the web client:
    • Think about app loading styles inlined in index.html.
  • If you changed core/:
    • Think about making types more precise. Can you better explain data relations by type?
    • Think about conflict resolution. We don’t need some very smart changing merging; just 2 changes of the same item on different clients should not break the database. What if the user changes an item on one machine and removes it on another?
    • Think about log and storage migration.
  • If you changed English translations:
    • Change translation ID if you change the meaning of the text.

core/download.ts Outdated Show resolved Hide resolved
core/loader/json-feed.ts Outdated Show resolved Hide resolved
@ai ai linked an issue Apr 13, 2024 that may be closed by this pull request
@ai
Copy link
Contributor

ai commented Apr 13, 2024

Sorry, we did small refactoring of loaders 😅

See:
#159
eafd288

Can you rebase?

core/loader/json-feed.ts Outdated Show resolved Hide resolved
core/download.ts Outdated Show resolved Hide resolved
core/loader/json-feed.ts Outdated Show resolved Hide resolved
@upteran upteran marked this pull request as ready for review April 15, 2024 11:56
core/loader/json-feed.ts Outdated Show resolved Hide resolved
core/loader/json-feed.ts Outdated Show resolved Hide resolved
core/loader/json-feed.ts Outdated Show resolved Hide resolved
core/loader/json-feed.ts Outdated Show resolved Hide resolved
core/loader/json-feed.ts Outdated Show resolved Hide resolved
@ai
Copy link
Contributor

ai commented Apr 15, 2024

Looks very good. If you don’t mind fixing code styling suggestion first, we can merge after that.

@ai
Copy link
Contributor

ai commented Apr 15, 2024

If you have a Twitter name, can you join us Discord and write me there? https://discord.gg/ZUV7JmN9

I want to write about JSON feed support.

@ai ai merged commit b77fe66 into hplush:main Apr 16, 2024
3 checks passed
@ai
Copy link
Contributor

ai commented Apr 16, 2024

A few ideas which came after the merge, which may be interesting for you 7f33ce0

  1. interface is a little faster than type
  2. satisafies a little better in these contexts, since it is arguments/keys that new types was not added
  3. I removed export type Item because Item is too generic name and could lead to name conflict.
  4. (value): boolean => can be replaced to just (value) => because ValidationRules already tells what is return value
  5. To render non-string value in console, you need to pass them as another argument

@ai
Copy link
Contributor

ai commented Apr 16, 2024

Amazing works, thanks!

@upteran
Copy link
Contributor Author

upteran commented Apr 16, 2024

If you have a Twitter name, can you join us Discord and write me there? https://discord.gg/ZUV7JmN9

I want to write about JSON feed support.

yep, @pezeze3 in twitter

thank you for feedback and ideas after, I'll check !

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.

Add JSON feed loader
2 participants