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

fix(sandbox): allow the sandbox to load plugins when running locally #367

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

demetriusfeijoo
Copy link
Contributor

@demetriusfeijoo demetriusfeijoo commented Mar 1, 2024

What?

Add the allowAllOrigins option to the createFieldPlugin function.

Why?

JIRA: EXT-2258

It allows our demo package (or any other plugin created by other developers) to be loaded in a local running instance of the Sandbox.

How to reproduce?

From the main branch run:

yarn
yarn dev:demo
yarn dev:container

Then, this error should appear in your console:
image

and the demo plugin should not be loaded:
image

How it should work after the change?

Now, from the task's branch, run these commands:

yarn
yarn dev:demo
yarn dev:sandbox

Then, no error should appear in your console:
image

and the demo plugin should be loaded:
image

PS: The Vercel action is not running yet since it wasn't modified to consider the new path (packages/sandbox) and also the new project name (modified here) in its settings.

Copy link

vercel bot commented Mar 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plugin-sandbox ❌ Failed (Inspect) Mar 1, 2024 11:45pm

@demetriusfeijoo demetriusfeijoo changed the title fix: allow the sandbox to load plugins when running locally fix(sandbox): allow the sandbox to load plugins when running locally Mar 1, 2024
@demetriusfeijoo demetriusfeijoo marked this pull request as ready for review March 2, 2024 00:42
host === 'plugin-sandbox.storyblok.com'
? 'https://plugin-sandbox.storyblok.com'
: 'https://app.storyblok.com'
allowAllOrigins === true
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would extract this for better code readability.

@BibiSebi
Copy link
Contributor

BibiSebi commented Mar 5, 2024

Thank you for suggesting this change @demetriusfeijoo.

I am wondering if we should add allowAllOrigins or something more explicit like whitelabeledOrigin so that it is never possible to allow all origins (*).

In any case, we need to add this to the docs explaining the usage of this new config parameter. What do you think?

@eunjae-lee
Copy link
Contributor

eunjae-lee commented Mar 5, 2024

Thank you for suggesting this change @demetriusfeijoo.

I am wondering if we should add allowAllOrigins or something more explicit like whitelabeledOrigin so that it is never possible to allow all origins (*).

In any case, we need to add this to the docs explaining the usage of this new config parameter. What do you think?

What if we even have something like this?

useFieldPlugin({ 
  target: 'https://something-else.com'
})

and if target is not provided, it falls back the the previous implementation (either sandbox or the production according to the host parameter)

@BibiSebi
Copy link
Contributor

BibiSebi commented Mar 5, 2024

@eunjae-lee, totally agree with your suggestion. @demetriusfeijoo However please deprioritize this PR for now due to the cycle commitment.

@demetriusfeijoo
Copy link
Contributor Author

Thanks for your reviews on this one @BibiSebi @eunjae-lee. 🚀

I thought about letting the developer use the wildcard (*) so, for example, even if the sandbox is running locally in a different port than 7000 (the default port for the Sandbox) the demo application wouldn't require any change. It would still work in that case.

When we return to this PR (after the cycle finishes) we may discuss having a third approach which would be your suggestion @eunjae-lee, but allowing also the wildcard, IDK.

Something like:

useFieldPlugin({ 
  target: 'https://something-else.com'
})

useFieldPlugin({ 
  target: '*'
})

If it's set to * we could then warn the developer in the console. 🤔

Just another idea that doesn't need to be handled now as we're at the begin of our cycle.

@demetriusfeijoo demetriusfeijoo marked this pull request as draft March 7, 2024 12:30
@demetriusfeijoo
Copy link
Contributor Author

I just realized that the Sandbox preview also doesn't work due to the same issue.

Base automatically changed from EXT-1470-replace-some-occurrences-of-container-with-sandbox to main March 7, 2024 12:48
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.

None yet

3 participants