Skip to content

Commit

Permalink
Correctly handle subpackage imports marked as external (#3407)
Browse files Browse the repository at this point in the history
* Correctly handle subpackage imports marked as external

* Remove the .only
  • Loading branch information
matthewp committed Jun 3, 2021
1 parent 1dac52b commit 64b377e
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 8 deletions.
22 changes: 14 additions & 8 deletions snowpack/src/sources/local.ts
Expand Up @@ -452,10 +452,8 @@ export class PackageSourceLocal implements PackageSource {
}

// Check to see if this package is marked as external, in which case skip the build.
for (const externalPackage of config.packageOptions.external) {
if (_packageName === externalPackage || _packageName.startsWith(externalPackage + '/')) {
return;
}
if(this.isExternal(_packageName, spec)) {
return;
}

await this.installPackage(_packageName, source);
Expand Down Expand Up @@ -684,10 +682,8 @@ export class PackageSourceLocal implements PackageSource {
const [packageName] = parsePackageImportSpecifier(spec);

// If this import is marked as external, do not transform the original spec
for (const externalPackage of config.packageOptions.external) {
if (packageName === externalPackage || packageName.startsWith(externalPackage + '/')) {
return spec;
}
if(this.isExternal(packageName, spec)) {
return spec;
}

const isSymlink = !entrypoint.includes(path.join('node_modules', packageName));
Expand Down Expand Up @@ -742,4 +738,14 @@ export class PackageSourceLocal implements PackageSource {
getCacheFolder() {
return this.cacheDirectory;
}

private isExternal(packageName: string, specifier: string): boolean {
const {config} = this;
for (const external of config.packageOptions.external) {
if (packageName === external || specifier === external || packageName.startsWith(external + '/')) {
return true;
}
}
return false;
}
}
40 changes: 40 additions & 0 deletions test/snowpack/config/packageOptions.external/index.test.js
Expand Up @@ -24,4 +24,44 @@ describe('packageOptions.external', () => {

expect(result['index.js']).toContain(`import 'fs';`);
});

it('Does not install externals that are deep package imports', async () => {
const result = await testFixture({
'packages/some-thing/main.js': dedent`
export default 'ok';
`,
'packages/some-thing/deep.js': dedent`
export default 'oops';
`,
'packages/some-thing/package.json': dedent`
{
"version": "1.0.0",
"name": "some-thing",
"module": "main.js"
}
`,
'package.json': dedent`
{
"version": "1.0.1",
"name": "@snowpack/test-config-external",
"dependencies": {
"some-thing": "file:./packages/some-thing"
}
}
`,
'index.js': dedent`
import 'some-thing';
import 'some-thing/deep.js';
`,
'snowpack.config.js': dedent`
module.exports = {
packageOptions: {
external: ['some-thing/deep.js']
}
}
`
});

expect(result['index.js']).toContain(`import 'some-thing/deep.js';`);
})
});

1 comment on commit 64b377e

@vercel
Copy link

@vercel vercel bot commented on 64b377e Jun 3, 2021

Choose a reason for hiding this comment

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

Please sign in to comment.