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.

Sign up for GitHub

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(common): pass manifest json to sandbox #273

Merged
merged 17 commits into from
Sep 22, 2023
Merged

feat(common): pass manifest json to sandbox #273

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
09ebd05
add manifest example in our templates
demetriusfeijoo Sep 18, 2023
561464f
read manifest file from templates
demetriusfeijoo Sep 18, 2023
d7c9b21
load manifest options from query-string
demetriusfeijoo Sep 18, 2023
6e131af
adjust tests
demetriusfeijoo Sep 18, 2023
122dc16
rename function for checking the existence of the manifest
demetriusfeijoo Sep 18, 2023
f2f3e38
remove unnecessary method
demetriusfeijoo Sep 18, 2023
ca9a4c6
remove unnecessary use o type 'any'
demetriusfeijoo Sep 18, 2023
6f19373
add manifest parameter only if necessary
demetriusfeijoo Sep 19, 2023
d5e1d10
Merge branch 'main' into EXT-1877-pass-manifest-json-to-sandbox
demetriusfeijoo Sep 21, 2023
a615ae1
remove all options from the manifest files
demetriusfeijoo Sep 21, 2023
90b59de
Merge branch 'EXT-1877-pass-manifest-json-to-sandbox' of github.com:s…
demetriusfeijoo Sep 21, 2023
acc9176
use chalk to style the logs
demetriusfeijoo Sep 21, 2023
80f9d32
Remove utf-8 explicity encoding since it's the default
demetriusfeijoo Sep 21, 2023
54fbea4
Merge branch 'EXT-1877-pass-manifest-json-to-sandbox' of github.com:s…
demetriusfeijoo Sep 21, 2023
4081224
return the explicity encoding
demetriusfeijoo Sep 21, 2023
2319e6e
add manifest schema validation
demetriusfeijoo Sep 21, 2023
5c0439a
improve readability when validating the options
demetriusfeijoo Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion packages/field-plugin/helpers/vite/src/manifest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,39 @@ export const load = (): Manifest => {
try {
const content: string = readFileSync(MANIFEST_FULL_PATH, 'utf8')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I kept informing the encoding since otherwise it would return me a Buffer instead of a string.

So, this way is the simplest one, I guess. If there is any other better way, let me know, please.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've had some other code like readFileSync(...).toString(). I don't know what's better, though.


return JSON.parse(content) as Manifest
const manifest: Manifest = JSON.parse(content)

validateSchema(manifest)

return manifest
} catch (err: any) {
throw new Error('Error while loading the manifest file: ' + err.message)
}
}

const validateSchema = (manifest: Manifest): void => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this simple validation, without using any external packages, here since we decided not to go too far on this.

const allowedProperties = ['options']

if (typeof manifest !== 'object') {
throw new Error(`The manifest should be an object`)
}

Object.keys(manifest).forEach((prop) => {
if (!allowedProperties.includes(prop)) {
throw new Error(`The property ${prop} is not allowed`)
}
})

if (manifest.options !== undefined && !Array.isArray(manifest.options)) {
throw new Error(`When declared, the 'options' property should be an array`)
}

manifest.options?.forEach((o) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename this o to option to be more informative. Other than that, it looks good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion here, @eunjae-lee. 🙌

I've just renamed it.

If there is something else, let me know, please.

if (!('name' in o && 'value' in o)) {
throw new Error(
`Some of the defined 'options' are invalid. ` +
`Please, make sure they contain a 'name' and 'value' properties`,
)
}
})
}