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: prevent error on not set location href #12494

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

mschakulat
Copy link
Contributor

Description

The current implementation of the function cleanScssBugUrl checks if the 'window' and 'location' globals are defined, and then replaces the prefix of the URL with the location href. However, in some cases, the 'location.href' value can be undefined, resulting in an error.

To resolve this issue, I've updated the code to use optional chaining (?.) to ensure that the 'replace' function only runs if 'location.href' is defined. This modification ensures that the function can handle cases where 'location.href' is undefined and prevents errors from being thrown.

This change has been tested and verified to work as expected.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented Mar 20, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@@ -1545,7 +1545,7 @@ function cleanScssBugUrl(url: string) {
typeof window !== 'undefined' &&
typeof location !== 'undefined'
) {
const prefix = location.href.replace(/\/$/, '')
const prefix = location.href?.replace(/\/$/, '')
Copy link
Member

Choose a reason for hiding this comment

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

I think we could replace 1546 with typeof location?.href === 'string'. If href isn't defined, then we don't need to call replace

Copy link
Member

Choose a reason for hiding this comment

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

@mschakulat you haven't fixed this part yet, which I agree would be a nicer check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bluwy Yes, that's correct. I have adjusted it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately condition must be typeof location !== 'undefined'.
In my project with svelte-kit and scss doesn't work new condition, because location is not defined.

Optional chaining doesn't work on not defined variables.

@patak-dev
Copy link
Member

patak-dev commented Mar 20, 2023

Would you give more info about your environment that is triggering this bug? Interesting that we didn't get a report before.

@mschakulat
Copy link
Contributor Author

mschakulat commented Mar 20, 2023

Would you give more info about your environment that is triggering this bug? Interesting that we didn't get a report before.

Unfortunately i can't say too much about it. We use a proprietary package (https://bitmovin.com/). The code is encrypted and therefore the context is hard to see. But I see that the 'location' object is edited there: typeof location?location:{}}.

@mschakulat mschakulat requested review from bluwy and patak-dev and removed request for patak-dev and bluwy March 21, 2023 07:55
@patak-dev patak-dev enabled auto-merge (squash) March 21, 2023 08:02
@patak-dev patak-dev merged commit 2fb8527 into vitejs:main Mar 21, 2023
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.

4 participants