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(compiler): handle null window.location.origin #2813

Conversation

brunor97
Copy link
Contributor

@brunor97 brunor97 commented Feb 5, 2021

when using a data URI or file URI (#2582), or when using iframe's srcdoc, the value for window.location.origin can be null. In these particular cases, passing null to new URL(url, null) will throw the following error: Failed to construct 'URL': Invalid base URL

when using a data URI or file URI (ionic-team#2582), or when using iframe's `srcdoc`, the value for window.location.origin can be `null`. In these particular cases, passing `null` to `new URL(url, null)` will throw the following error: Failed to construct 'URL': Invalid base URL
@00salmon
Copy link

+1, currently blocked by this when using components in an iframe

@brunor97
Copy link
Contributor Author

+1, currently blocked by this when using components in an iframe

As a hotfix, I'm using patch-package to apply this fix on my project's side, while we wait for this PR to be merged and published

Not a long term solution but hopefully this can unblock you 😃

@00salmon
Copy link

+1, currently blocked by this when using components in an iframe

As a hotfix, I'm using patch-package to apply this fix on my project's side, while we wait for this PR to be merged and published

Not a long term solution but hopefully this can unblock you 😃

that's more convenient than editing the file in dist directly before each publish, thank you!

@brunor97
Copy link
Contributor Author

+1, currently blocked by this when using components in an iframe

As a hotfix, I'm using patch-package to apply this fix on my project's side, while we wait for this PR to be merged and published
Not a long term solution but hopefully this can unblock you 😃

that's more convenient than editing the file in dist directly before each publish, thank you!

No problem! Glad I could be of help 😄

@Sanderand
Copy link

Sanderand commented Jul 27, 2021

+1, we make use of the patch-package solution for this issue. for @Stencil@[email protected] the patch file is 8.5mb and needs to be checked into our repo. 🥲

will this PR eventually be considered?

@brunor97 brunor97 requested a review from a team September 14, 2021 10:56
@loganvolkers
Copy link

Bump @rwaskiewicz

If this needs some changes to get it over the line, please let me know. We have dozens of stencil projects that all need this fix manually applied so they are compatible with sandboxed iframes and srcdoc iframes

@loganvolkers
Copy link

e.g. I'm happy to add some test cases to this PR if that will help it get across the line

@jan-paulus
Copy link
Contributor

Any update on this?

1 similar comment
@Sanderand
Copy link

Any update on this?

@leonheess
Copy link

@rwaskiewicz, please merge these changes or outline any requirements that need to be fulfilled

@jan-paulus
Copy link
Contributor

@rwaskiewicz Could you take a look?

We've used patch-package to workaround this issue for more than a year now.

Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

LGTM - I have one stylistic nitpick that I think can be applied in GH

src/compiler/output-targets/dist-lazy/generate-system.ts Outdated Show resolved Hide resolved
Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

I'm going to be merging this as an admin - the failing CI step is a known issue when running against forks. The step in question is our tech debt burndown chart, which I'm able to see the output of (no new unused functions or strict null checks violations are added in this PR)

@rwaskiewicz rwaskiewicz merged commit 255cd66 into ionic-team:main Jun 23, 2022
@rwaskiewicz rwaskiewicz added Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. Bug: Validated This PR or Issue is verified to be a bug within Stencil and removed Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. labels Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants