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

support custom web modules dir #483

Merged
merged 3 commits into from Jun 12, 2020
Merged

support custom web modules dir #483

merged 3 commits into from Jun 12, 2020

Conversation

danprince
Copy link
Contributor

Allows for installing modules into custom directories with the following script:

"scripts": {
  "mount:web_modules": "mount $WEB_MODULES --to /wherever_you_want"
}

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 whatever web_modules dir we pull out of the config. Would that scanning code also need to scan for the baseUrl 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 for webModulesLoc or are there other considerations?

Closes #441

@danprince danprince requested a review from a team as a code owner June 11, 2020 22:56
const dirUrl = to || `/${cmdArr[0]}`;
if (scriptId === 'mount:web_modules') {
dirDisk = dependenciesLoc;
}
Copy link
Collaborator

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?

Copy link
Contributor Author

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);
Copy link
Collaborator

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' }
});
Copy link
Collaborator

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.

Copy link
Collaborator

@drwpow drwpow left a 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.

@FredKSchott
Copy link
Owner

Likewise, LGTM! I'll make a note in #475 to resolve the final piece inside of scanImports, but that work shouldn't block this PR.

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') {
Copy link
Contributor Author

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.

Copy link
Owner

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

@FredKSchott FredKSchott merged commit 711a816 into FredKSchott:master Jun 12, 2020
@danprince danprince deleted the custom-web-modules-dir branch April 21, 2022 11:01
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.

Support custom directory for installed dependencies
3 participants