-
Notifications
You must be signed in to change notification settings - Fork 916
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 Svelte plugin SSR bug; add test #1241
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/4vh43ujip |
const userSvelteConfigLoc = path.join(process.cwd(), 'svelte.config.js'); | ||
// Note(drew): __config is for internal testing use; maybe we should make this public at some point? | ||
const userSvelteConfigLoc = | ||
pluginOptions.__config || path.join(process.cwd(), 'svelte.config.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a config option so that the preprocessor could be tested. Should this be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, not opposed to supporting a simple JSON object (or JS object) via plugin options, in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Yeah let me think about it (I actually had trouble finding out about svelte.config.js
being a thing)
|
||
it('passes options to compiler', () => { | ||
const options = { | ||
generate: 'ssr', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main bug we’re testing for: generate: 'ssr'
wasn‘t being passed to the plugin. This test failed before the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 1 small comment
const userSvelteConfigLoc = path.join(process.cwd(), 'svelte.config.js'); | ||
// Note(drew): __config is for internal testing use; maybe we should make this public at some point? | ||
const userSvelteConfigLoc = | ||
pluginOptions.__config || path.join(process.cwd(), 'svelte.config.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, not opposed to supporting a simple JSON object (or JS object) via plugin options, in a follow up PR.
d9ec0ef
to
049afab
Compare
049afab
to
99bbe6c
Compare
99bbe6c
to
9c4ff53
Compare
9c4ff53
to
bdef3c5
Compare
The new tests currently seem to fail on Windows: |
…it found (#1233) * writing the first docs test and fixing the errors it found * Creating a GitHub Action * Update docs.yml * fixes to the test runner and actions * removing jest from docs
Changes
Fixes the SSR bug introduced in #1221 where a user’s
generate
settings were always overridden. Adds a test to help with regressions.Testing
Tests were added! Tests should pass.
Testing this manually is a bit arduous; please comment if you’d like a walkthrough of this setup.
Docs
No docs needed; bugfix + test only