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: avoid poisoning global scope #457

Merged
merged 1 commit into from Mar 31, 2023
Merged

fix: avoid poisoning global scope #457

merged 1 commit into from Mar 31, 2023

Conversation

Ayc0
Copy link
Contributor

@Ayc0 Ayc0 commented Mar 31, 2023

  • Add a test if possible
    I didn't add any automated test, but I checked that after running pnpm build, there was no mention of Window in the index.d.ts file:
    image
    I didn't add any test because:
    • this test would require to be ran after pnpm build
    • if we wanted to write to test to ensure that we aren't poisoning the global scope, we need to do something like "is Response, or Window, etc., in the generated code?" But Response is used so we cannot use that, and other may be used in the future. So I don't know which test we could write that could be future proof for this.
  • Format all commit messages with Conventional Commits
  • This PR fixes Global fetch type is getting modified by shiki #454

I couldn't use just declare var fetch and others, because we need to add to the global scope Window and others as they are used by shiki's node_modules (and so when running tests locally, we need them, see https://github.com/shikijs/shiki/actions/runs/4572982584/jobs/8072860830).

So I moved those to a global.d.ts so that they'd be present in the project, but not in the built code.

@netlify
Copy link

netlify bot commented Mar 31, 2023

Deploy Preview for shiki-matsu ready!

Name Link
🔨 Latest commit d6f9557
🔍 Latest deploy log https://app.netlify.com/sites/shiki-matsu/deploys/64269eced41707000764e484
😎 Deploy Preview https://deploy-preview-457--shiki-matsu.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Ayc0 Ayc0 changed the title fix: avoid using declare global to avoid poising global scope fix: avoid using declare global to avoid poisoning global scope Mar 31, 2023
@Ayc0 Ayc0 marked this pull request as ready for review March 31, 2023 08:54
@Ayc0 Ayc0 changed the title fix: avoid using declare global to avoid poisoning global scope fix: avoid poisoning global scope Mar 31, 2023
@orta
Copy link
Contributor

orta commented Mar 31, 2023

Yeah, I think this should do it 👍🏻

@orta orta merged commit b0a4039 into shikijs:main Mar 31, 2023
2 checks passed
@orta
Copy link
Contributor

orta commented Mar 31, 2023

Nice work!

@Ayc0 Ayc0 deleted the Ayc0/fix-globals branch March 31, 2023 10:09
@Ayc0
Copy link
Contributor Author

Ayc0 commented Mar 31, 2023

Thanks!

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

2 participants