-
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
support custom web modules dir #483
support custom web modules dir #483
Conversation
const dirUrl = to || `/${cmdArr[0]}`; | ||
if (scriptId === 'mount:web_modules') { | ||
dirDisk = dependenciesLoc; | ||
} |
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.
nit: I know the code that was here before was a little confusing (at least to me), and you changed as little as possible. But with this addition do you think you could add a comment or two to explain what this section is doing?
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.
Yes! Hadn't seen too many existing comments in the src
files and had been a bit hesitant to start adding them. Hopefully that's clearer now!
}); | ||
|
||
// should write modules to the custom folder on disk | ||
expect(fs.existsSync(path.join(__dirname, 'build', 'my_modules', 'array-flatten.js'))).toBe(true); |
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.
Nice! 👍
// override NODE_ENV=test from jest, otherwise snowpack will assume | ||
// development mode and try to copy from DEV_DEPENDENCIES_DIR | ||
env: { NODE_ENV: 'production' } | ||
}); |
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.
Nice catch! And good comment here.
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 looks good! Happy to merge whenever you’re ready.
Likewise, LGTM! I'll make a note in #475 to resolve the final piece inside of Thanks for taking a stab at this |
@@ -357,9 +357,13 @@ function normalizeScripts(cwd: string, scripts: RawScripts): BuildScript[] { | |||
} | |||
let dirDisk = cmdArr[0]; | |||
const dirUrl = to || `/${cmdArr[0]}`; | |||
|
|||
// mount:web_modules is a special case script where the fromDisk | |||
// arg is harcoded to match the internal dependency dir | |||
if (scriptId === 'mount:web_modules') { |
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.
@FredKSchott What do you think about doing this instead?
dirDisk = dirDisk.replace('$WEB_MODULES', dependenciesLoc);
That would make the Script Variables section of the docs more accurate and technically it'd be a little bit more flexible. Not sure that anyone needs/wants that flexibility though.
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.
+1, I think that's a good idea. Feel free to tackle in a future PR
Allows for installing modules into custom directories with the following script:
In both dev and build dependencies would be moved to
/wherever_you_want
rather than the traditional/web_modules
folder.@FredKSchott Might need a hand getting in the
scan-imports.ts
bit of this over the line.Do the comments in #475 mean that we're keeping the token scanning? If so, then there needs to be some restructuring so that
parseWebModuleSpecifier
(and all of its callers) have access to whateverweb_modules
dir we pull out of the config. Would that scanning code also need to scan for thebaseUrl
as part of the token too?Other than that it looks like it's just the
scanImports
function that needs a couple of updates. Is that as simple as just swapping references to"web_modules"
out forwebModulesLoc
or are there other considerations?Closes #441