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

feat(overlay): Allow to skip sidecar #501

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

mydea
Copy link
Member

@mydea mydea commented Aug 21, 2024

For the frontend-only use-case, we do not have a sidecar, so we want to skip connecting to it & do not show a warning for that.

ref #133

@mydea mydea requested review from BYK and Lms24 August 21, 2024 09:23
@mydea mydea self-assigned this Aug 21, 2024
Copy link

vercel bot commented Aug 21, 2024

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

Name Status Preview Comments Updated (UTC)
spotlightjs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 21, 2024 10:59am

mydea pushed a commit that referenced this pull request Aug 21, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and publish to npm
yourself or [setup this action to publish
automatically](https://github.com/changesets/action#with-publishing). If
you're not ready to do a release yet, that's fine, whenever you add more
changesets to fn/prerelease-addEnvelope, this PR will be updated.

⚠️⚠️⚠️⚠️⚠️⚠️

`fn/prerelease-addEnvelope` is currently in **pre mode** so this branch
has prereleases rather than normal releases. If you want to exit
prereleases, run `changeset pre exit` on `fn/prerelease-addEnvelope`.

⚠️⚠️⚠️⚠️⚠️⚠️

# Releases
## @spotlightjs/[email protected]

### Minor Changes

-   feat(overlay): Allow to skipSidecar to avoid connecting to sidecar
    ([#501](#501))

## @spotlightjs/[email protected]

### Patch Changes

-   Updated dependencies \[]:
    -   @spotlightjs/[email protected]

## @spotlightjs/[email protected]

### Patch Changes

-   Updated dependencies

\[[`0ec320d1259c85e3505d6a80f88dc41d0f567d9e`](0ec320d)]:
    -   @spotlightjs/[email protected]

## @spotlightjs/[email protected]

### Patch Changes

-   Updated dependencies

\[[`0ec320d1259c85e3505d6a80f88dc41d0f567d9e`](0ec320d)]:
    -   @spotlightjs/[email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mydea pushed a commit that referenced this pull request Aug 21, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and publish to npm
yourself or [setup this action to publish
automatically](https://github.com/changesets/action#with-publishing). If
you're not ready to do a release yet, that's fine, whenever you add more
changesets to fn/prerelease-addEnvelope, this PR will be updated.

⚠️⚠️⚠️⚠️⚠️⚠️

`fn/prerelease-addEnvelope` is currently in **pre mode** so this branch
has prereleases rather than normal releases. If you want to exit
prereleases, run `changeset pre exit` on `fn/prerelease-addEnvelope`.

⚠️⚠️⚠️⚠️⚠️⚠️

# Releases
## @spotlightjs/[email protected]

### Patch Changes

-   Updated dependencies \[]:
    -   @spotlightjs/[email protected]

## @spotlightjs/[email protected]

### Patch Changes

-   Updated dependencies

\[[`28818193ba34093d456bd855c483eb61e364d14d`](2881819)]:
    -   @spotlightjs/[email protected]

## @spotlightjs/[email protected]

### Patch Changes

- fix(overlay): Skip contextLines lookup if sidecar is empty
([#501](#501))

## @spotlightjs/[email protected]

### Patch Changes

-   Updated dependencies

\[[`28818193ba34093d456bd855c483eb61e364d14d`](2881819)]:
    -   @spotlightjs/[email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@BYK
Copy link
Member

BYK commented Aug 21, 2024

This change looks a bit invasive. Do we really need a skipSidecar option? Like how about we pass sidecarUrl: false or even better, we just say we cannot connect to sidecar but we can run in FE-only mode?

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Blocking till we settle on the way forward. Otherwise changes look OK.

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.

3 participants