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

[chore] resolve outdated todos #2210

Merged
merged 3 commits into from
Aug 15, 2021
Merged

[chore] resolve outdated todos #2210

merged 3 commits into from
Aug 15, 2021

Conversation

ignatiusmb
Copy link
Member

Resolve some outdated TODOs that we can safely remove as we're approaching 1.0

@changeset-bot
Copy link

changeset-bot bot commented Aug 15, 2021

⚠️ No Changeset found

Latest commit: 602a1db

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

nice cleanup!

@benmccann benmccann merged commit b772016 into sveltejs:master Aug 15, 2021
@ignatiusmb ignatiusmb deleted the chore/todos branch August 15, 2021 13:15
@@ -1,7 +1,6 @@
import { getContext } from 'svelte';

// const ssr = (import.meta as any).env.SSR;
const ssr = typeof window === 'undefined'; // TODO why doesn't previous line work in build?
const ssr = import.meta.env.SSR;
Copy link

@wallw-teal wallw-teal Aug 18, 2021

Choose a reason for hiding this comment

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

This broke some test mocks on our end. What's the best way to get import.meta.env to be defined in a non-vite context such as Jest or another test runner?

Edit: That's with a setup similar to this example except I think ours is using jest's esm support.

Overall not a huge deal, as there are multiple workarounds. Ideally there would be a project similar to vite-jest that would spin up Svelte Kit. I was just wondering if there was a recommended approach.

Copy link
Member

Choose a reason for hiding this comment

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

there's an issue about this. it's been one of the things to figure out before 1.0: #1485

Copy link
Contributor

Choose a reason for hiding this comment

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

@wallw-bits @benmccann

I noticed this breaking tests for me as well - I think something like

import { browser } from './env.js'

const ssr = !browser

would do the trick, as then you can just mock $app/env.js with

jest.mock('$app/env.js', () => ({
  amp: false,
  browser: true, // or false for testing ssr
  dev: true,
  mode: 'test'
}))

Copy link
Contributor

Choose a reason for hiding this comment

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

just tried it - making that change locally did work

image

I think it also makes the code more DRY - import.meta.env.SSR is already being dealt with in env.

I'll make a PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR made: #2353

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