Add Typescript support, export only components #20
Conversation
There was a problem hiding this comment.
Thanks for this! The new dev workflow is awesome.
I had some trouble getting this to work on my static site and wasn't sure if you dropped support for that usage or whether it's just that I don't understand JS tooling well enough to know how to do it. But if that did get dropped, I think we should undo it, because I believe that's the most common usage at the moment and I want this library to support non-react sites. If it didn't get dropped can you provide some instructions on how to do it?
Other than that I sprinkled a few other comments around. I haven't wrapped my head around the vite config changes yet, but wanted to wait till I had a working version on my old dev sites before digging into those more.
Agree an updated README would be very helpful once we've finalized a new API.
| import { PostSummary } from './PostSummary'; | ||
| import { Comment } from './Comment'; | ||
|
|
||
| type Reply = { |
There was a problem hiding this comment.
these just weren't used? is that right?
There was a problem hiding this comment.
That's right ... since we're using AppBskyFeedDefs.ThreadViewPost for our thread now, we don't need to have these.
;)
| @@ -1,5 +1,5 @@ | |||
| import React from 'react'; | |||
| import { AppBskyFeedDefs, AppBskyFeedPost } from "@atproto/api"; | |||
| import { AppBskyFeedDefs, AppBskyFeedPost } from '@atproto/api'; | |||
There was a problem hiding this comment.
sorry, probably a dumb question but what formatter did you use for this? and is there an easy way for me/everyone to adopt it?
There was a problem hiding this comment.
I think that's just how I've got VSCode set up.
Moving forward I'd recommend adding Prettier and ESLint to the project which would help with (a) formatting and (b) assist with finding/fixing any issues during development.
Happy to help with that!
| export { CommentSection } from './CommentSection' | ||
| export { Filters } from './CommentFilters' | ||
| export { CommentSection as BlueskyComments } from './CommentSection'; | ||
| export { Filters as BlueskyFilters } from './CommentFilters'; |
There was a problem hiding this comment.
Out of curiosity what was your motivation for renaming these? And if we're going to rename them in the API should we not just rename the components too?
There was a problem hiding this comment.
Motivation being that if I were using this in my project I'd prefer seeing/using a component named BlueskyComments vs. CommentSection.
Yes, I do think the components should be renamed!
I didn't want to do that just yet so as to make the initial PR readable/understandable from a "diff" perspective. Easier to see the changes in an existing file vs. a new file.
There was a problem hiding this comment.
Ok cool. Yeah that seems like a reasonable plan. So we can rename it in a later PR.
| import type { CommentOptions } from './types' | ||
|
|
||
| export const BlueskyComments = { | ||
| init: function(elementId: string, options: CommentOptions) { |
There was a problem hiding this comment.
Do we not need still want this init function for the case where people are just dropping the script via a CDN on a static page? Is there some other way to do that after this change?
There was a problem hiding this comment.
Personally I don't see a use case for this given that in the end it is a React component.
If I wasn't using React, I'd reach for a different solution:
https://github.com/ascorbic/bluesky-comments-tag
There was a problem hiding this comment.
Hmm, that does look like a nice option, though I don't think it supports the user-based mode? Or filters, etc.
Is there any issue with leaving this option in while also getting the improvements here added? As someone with a blog that is already using the library this way I don't want to have to have to change solutions (especially since I'm not sure any other options have feature parity with this one).
There was a problem hiding this comment.
Ok, I was able to get this working so I think we can move forward on this path
|
|
||
| // Support both module exports and window global | ||
| // Add to window object for UMD builds | ||
| if (typeof window !== 'undefined') { |
There was a problem hiding this comment.
i'm ok dropping support for the window version if there's still an easy way to set it up outside a native react environment, but also is there any harm in leaving this in?
There was a problem hiding this comment.
As stated above, I don't see the use case for it. The contents of /dev does what the init function was doing; an HTML file that loads in a TS (or JS) file which itself handles the imports and renders the component.
This is lieu of adding the CSS, adding React/ReactDOM, adding the event listener for DOMContentLoaded, and adding the necessary div.
|
I think you should be able to use the contents of |
Oops responded to your other comment before processing this. Let me play with this a bit and see if I can get it working on my own site without any additional tooling. |
|
Ok, I got this working like this. That's not so bad compared to the current instructions. <script type="importmap">
{
"imports": {
"react": "https://esm.sh/react@18",
"react-dom": "https://esm.sh/react-dom@18",
"react-dom/client": "https://esm.sh/react-dom@18/client"
}
}
</script>
<script type="module">
import { createElement } from 'react';
import { createRoot } from 'react-dom/client';
import {BlueskyComments, BlueskyFilters} from '/assets/bluesky-comments/bluesky-comments.es.js';
const uri = '{{ page.bluesky_post_uri }}';
const author = 'coryzue.com';
const container = document.getElementById('bluesky-comments');
const root = createRoot(container);
root.render(
createElement(BlueskyComments, {
author: author,
uri: uri,
commentFilters: [
BlueskyFilters.NoPins,
BlueskyFilters.MinCharacterCountFilter(10)
]
})
);
</script> |
Exports only components and removes the need for the
initfunction for a React component/application.Removes the extraneous Vite config file and adds a
dev/directory for development of features.Dev
npm run devTODO