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

bugfix: Allow baseUrl to be remote #531

Merged
merged 1 commit into from
Jun 23, 2020
Merged

bugfix: Allow baseUrl to be remote #531

merged 1 commit into from
Jun 23, 2020

Conversation

drwpow
Copy link
Collaborator

@drwpow drwpow commented Jun 23, 2020

If a user has a remote URL (https://cdn.pika.dev) as either their homepage or their buildOptions.baseUrl, we accidentally ruin that import with path.posix.join() (https://cdn.pika.dev becomes https:/cdn.pika.dev). This fixes that.

@drwpow drwpow requested a review from a team as a code owner June 23, 2020 17:33
@vercel
Copy link

vercel bot commented Jun 23, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/okkch6346
✅ Preview: https://snowpack-git-drwpow-remote-base-url.pikapkg.vercel.app

@drwpow drwpow requested a review from FredKSchott June 23, 2020 17:39
@drwpow drwpow changed the title Allow baseUrl to be remote bugfix: Allow baseUrl to be remote Jun 23, 2020
<html lang="en">
<head>
<meta charset="utf-8" />
<link rel="icon" href="https://cdn.pika.dev/favicon.ico" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👆 proof it’s working. That would have been https:/cdn.pika.dev otherwise

@@ -0,0 +1,3 @@
import {flatten} from "https://cdn.pika.dev/web_modules/array-flatten.js";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👆 here, too

src/commands/import-resolver.ts Outdated Show resolved Hide resolved
src/commands/import-resolver.ts Outdated Show resolved Hide resolved
test/build/base-url-remote/package.json Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@ import {SnowpackConfig} from '../config';
import {findMatchingMountScript, ImportMap} from '../util';
import srcFileExtensionMapping from './src-file-extension-mapping';
const cwd = process.cwd();
const URL_HAS_PROTOCOL_REGEX = /^\w:\/\./;
const URL_HAS_PROTOCOL_REGEX = /^(\w+:)?\/\//;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just realized there was a bug in this RegEx. \w matches a word character, not a word. So \w+ is needed.

I also removed the character trailing the // (the .). I think that’s OK, because this was only used in a .test() below? I needed to remove that to strip just the protocol, not the protocol + 1 trailing char below.

@@ -77,13 +77,18 @@ export function createImportResolver({
return spec;
}
if (dependencyImportMap && dependencyImportMap.imports[spec]) {
// if baseURL is remote, handle that outside of path.posix.join()
const protocolMatch = config.buildOptions.baseUrl.match(URL_HAS_PROTOCOL_REGEX);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn’t notice the existing RegEx here. Reused what was here, with the bugfix (above)

@drwpow drwpow merged commit 5e6fc22 into master Jun 23, 2020
@drwpow drwpow deleted the drwpow/remote-base-url branch June 23, 2020 19:24
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.

None yet

3 participants