Add preview command support #21
Conversation
…etching web domains, modularize routes a bit
api/preview.js
Outdated
| }); | ||
| } | ||
|
|
||
| const fetchContentMetadata = async ( |
There was a problem hiding this comment.
Much of this ripped straight from cms-js-platform
|
|
|
With the preview command leveraging some of the watch commands logic in some ways -- have you considered refactoring the watch and preview commands to DRY up the code in the lib? Taking a look at both, its definitely not something that we need to do initially for MVP because I wouldn't want to get too in the weeds with touching the currently working watch command -- however, it would be best if we didn't have these two commands diverge in their "watching/uploading" functionality if one is changed (the local filesystem aspect of it), the other should (seemingly) behave the same to the developer -- with the nuances of each command built into their own separate areas. |
Oops, missed this - I think I have it as a global so it wasn't erroring for me 🥲
Definitely something that has crossed my mind. I think for the time being the best approach might be to keep it separate. There's going to be some unique functionality we want to throw in there to synchronize rendering / uploading stuff, and I wasn't certain about how uploading to the preview buffer would look compared to our normal upload (though it's looking like we may be able to use the same method of uploading, just changing the path. I do think down the line it might make sense to unify them though, or least clean up |
Definitely agree -- Waiting to see how this shapes up is best before deciding to refactor is a good call initially. We can follow up on it after MVP -- as there might be more feedback and quick changes that we need to adopt without thinking how that is going to affect the standard |
Co-authored-by: Tanya Scales <[email protected]>
Changes pre-ET
|
The code in here looks reasonable to me, but tbh it's too much code for me to be able to confidently ✅ We had previously discussed porting this logic into local-dev-lib, but I think we may want to reconsider that and instead work towards the pattern being used by the JS rendering and UI extensions teams. The local-dev-lib library is intended to contain common logic that gets used in many places. Our current pattern for large, single-use features like this dev server is to offer them as their own npm package. Then the CLI can include the package as a dependency. This would give us all a few benefits:
Edit: Just to clarify, I don't think doing this ^ should prevent you from being able to merge this PR. We should just chat about it before you get started porting any of this into the local-dev-lib. |
|
Everything's looking good as far as I can tell. I defer to Tanya for all the net new stuff, but as far as backwards compatibility goes I think we're all good |
| } catch (err) { | ||
| logger.error(`Failed to fetch module preview for ${calculatedPath}`) | ||
| } | ||
| if (!customWidgetInfo || !('moduleId' in customWidgetInfo && 'previewKey' in customWidgetInfo)) { |
There was a problem hiding this comment.
This seems very brittle--do we have plans to refactor this in the future?
There was a problem hiding this comment.
Can you explain what you mean by brittle here? I may be too frog-in-boiling-water to see anything iffy 👀
There was a problem hiding this comment.
I left some comments directly in the code, where I spotted some possible bugs. I also wanted to ask why we've written a proxy server, when there is express middleware that creates proxies in a single line. Do we need specialized functionality? I agree with @brandenrodgers that we should revisit the code and consider releasing this as its own npm package.
|
I tested locally and things seem to be working! The only (potential) issue is that I saw this error page when clicking on one of my domains: Maybe that's WAD though? |
Hadn't considered using an extra middleware for it - there is some special handling that takes place to convert our special URL format into the actual preview call that needs to happen but I see there's a URL builder sort of feature in there - definitely gonna explore whether it's worth moving that over before launch 👍
Does that domain / path exist as a page on your portal? We do a straightforward fetch to the URL provided with some added params to get the preview render so if you're getting that error then it means that call failed |
|
Also +1 on making it its own separate package when we do go to release it - definitely large enough that that's warranted 👍 |
See PR description for HubSpot/hubspot-cli#898