-
Notifications
You must be signed in to change notification settings - Fork 354
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(api): refactor sandpack provider #342
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0a5a9ef:
|
Size changessandpack-react
Details
sandpack-client
Details
|
4d57251
to
dac3a1f
Compare
83beb12
to
23c1083
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice refactor, found some small typos and a question about file path normalization
} | ||
const getPackageJsonFile = (): string | undefined => { | ||
if (newFiles["/package.json"]) return "/package.json"; | ||
if (newFiles["package.json"]) return "package.json"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we normalise these paths at some point? Just a note for potential future improvements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed we need to address it. I noted this problem when I was trying to implement a new feature, which fetches sandboxes from CodeSandbox, and I faced a ton of issues (see resolveFile
for instance).
Here's the task: #353
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
const strategies = [".js", ".jsx", ".ts", ".tsx"]; | ||
const leadingSlash = Object.keys(files).every((file) => file.startsWith("/")); | ||
|
||
while (!resolvedPath && index < strategies.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would benefit from some comments for future proofness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thanks for mentioning it. I'll revisit it later in this task, there is still room for improvements there
const leadingSlash = Object.keys(files).every((file) => file.startsWith("/")); | ||
|
||
while (!resolvedPath && index < strategies.length) { | ||
const slashPath = (): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I would extract this as a general util for strings/paths
@@ -184,42 +183,6 @@ experience by modifying the `recompileDelay` value or by setting the | |||
/> | |||
``` | |||
|
|||
## Sandpack Runner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe keep the docs about Sandpack Runner with a mention that it was deprecated and an example of how to use the Preview to get the same result. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. But I would only add an example of how to use the Preview and leave to mention the deprecated component in the Migration Guide. I'll add to my to-do list to address all the documentation parts at once.
|
||
/** | ||
* @category Presets | ||
*/ | ||
export const Sandpack: React.FC<SandpackProps> = (props) => { | ||
// Combine files with customSetup to create the user input structure | ||
const userInputSetup = props.files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels good to get rid of this kind of logic :)
openPaths: props.options?.openPaths, | ||
activePath: props.options?.activePath, | ||
recompileMode: props.options?.recompileMode, | ||
recompileDelay: props.options?.recompileDelay, | ||
autorun: props.options?.autorun ?? true, | ||
autorun: props.options?.autorun, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't searched for this, but doesn't the autorun need to be true
by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I leave the SandpackContext to set the default values
customSetup={userInputSetup} | ||
customSetup={props.customSetup} | ||
files={props.files} | ||
options={providerOptions} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 this looks so much better now
This PR aims to solve a few inconsistencies and redundancies in the surface API for the main components (Sandpack and SandpackProvider). Although I could take this opportunity to introduce more changes in the organization of files, I would rather do only the necessary changes and spent more time writing tests.
It's important to point out that with #276 and #326, this PR will trigger a major bump in the versions of the packages.
Breaking changes:
SandpackPreview
component;Sandpack
API, except that for some exceptions;customSetup.main
has been deprecated: due to redundancy, useoptions.activePath
instead;customSetup.files
has been deprecated: due to redundancy, usefiles
instead;Example:
Others features
getSandpackStateFromProps
, which is one of the core utility in Sandpack;files
;A migration guide will be addressed in another situation.
Closes #325 closes #329