Add buildOptions.metaDir option #456
Conversation
| "test": "jest test/integration/runner.js --test-timeout=15000 --testMatch \"**/test/**/*.[jt]s?(x)\" -i", | ||
| "test": "npm run test:integration && npm run test:build", | ||
| "test:build": "jest test/build", | ||
| "test:integration": "jest test/integration/runner.js --test-timeout=15000 --testMatch \"**/test/**/*.[jt]s?(x)\" -i", |
There was a problem hiding this comment.
❓Is this a good change to allow for more dev & build tests in the future?
| hasHmr = false, | ||
| ) { |
There was a problem hiding this comment.
I personally like converting function params to an option whenever it gets > 4, especially when some are optional. I never can remember the ordering 😅
| code, | ||
| hasHmr = false, | ||
| buildOptions, | ||
| }: { |
There was a problem hiding this comment.
Another example of ordered params -> option
There was a problem hiding this comment.
Yea with just JS I'd 100% agree, with TS types I'm ambivalent but overall I'm 👍 if it helps others
| @@ -113,6 +113,9 @@ export interface SnowpackConfig { | |||
| dedupe?: string[]; | |||
| }; | |||
| }; | |||
| buildOptions: { | |||
| metadataDir: string; | |||
| }; | |||
There was a problem hiding this comment.
The core code change 👆. Everything else is just implementation.
| // remove leading/trailing slashes | ||
| config.buildOptions.metadataDir = config.buildOptions.metadataDir | ||
| .replace(/^(\/|\\)/g, '') // replace leading slash | ||
| .replace(/(\/|\\)$/g, ''); // replace trailing slash |
There was a problem hiding this comment.
Also see here: I figured a lot of people would try /static/snowpack/ as the option, and wanted to handle that here.
drwpow
force-pushed
the
drwpow/metadata-folder
branch
3 times, most recently
from
25036ca to
60bd5b2
Compare
| @@ -50,6 +50,7 @@ $ snowpack dev --no-bundle | |||
| "scripts": { /* ... */ }, | |||
| "installOptions": { /* ... */ }, | |||
| "devOptions": { /* ... */ }, | |||
| "buildOptions": { /* ... */ }, | |||
There was a problem hiding this comment.
+1, plenty of things in devOptions should really be moved to buildOptions. It would be a good follow-up to this PR to migrate the relevant ones over from devOptions -> buildOptions, if we could continue to support the legacy devOptions usage in our normalizeConfig() function.
src/commands/build.ts
Outdated
| url: proxiedUrl, | ||
| code: proxiedCode, | ||
| ext: proxiedExt, | ||
| buildOptions: config.buildOptions, |
There was a problem hiding this comment.
I have a slight preference for passing around the full config option whenever we need access to a full slice of config, instead of destructuring different parts of it. Especially in TypeScript, it's less mental overhead to only keep track of a single config type.
Would also be +1 with just passing the single metaDir string as an argument instead of any config objects.
Will leave it up to you how to resolve, if you have strong feels either way.
There was a problem hiding this comment.
Yeah passing the full config makes sense. I did think about the individual metaDir string, but I wanted to keep the mental mapping to where that comes from, as well as reduce noise if something else was needed from config. To that end the full config makes more sense!
| const path = require('path'); | ||
| const execa = require('execa'); | ||
|
|
||
| it('buildOptions.metadataDir', () => { |
There was a problem hiding this comment.
fine for our first test, but our 2nd/3rd test should move this into a runner similar to test/integrations so that we can standardize the test structure.
There was a problem hiding this comment.
Yeah totally agree. I just didn’t have a good feel for what that runner should be; hopefully a few more tests in this direction will make that clearer.
drwpow
force-pushed
the
drwpow/metadata-folder
branch
3 times, most recently
from
8ac9215 to
d50c9d6
Compare
drwpow
force-pushed
the
drwpow/metadata-folder
branch
from
d50c9d6 to
39e5252
Compare
drwpow
changed the title
Reason for Change
Addresses #442 with a new config option (and a new
buildOptionsnamespace):Feedback
The option name
I’m 🍌 (unopinionated) on the
metaDirname itself. I wanted something more specific than “meta,” or “metadata“ and didn’t know what else to call it.But
buildOptionsfelt like a logical namespace alongsideinstallOptionsanddevOptions. I simply included it here, and notdevOptions, because it does exist mostly at thebuildcommand.Test additions
This adds a new
/test/buildfolder to testsnowpack build. I felt that the existing integration tests that try and look for the sameexpected-installandexpected-output.txtstructure might be weird and preserve.