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

Allow OnResolve plugins to mark modules as side effect free #1313

Merged
merged 6 commits into from Jun 8, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/esbuild/service.go
Expand Up @@ -715,6 +715,9 @@ func (service *serviceType) convertPlugins(key int, jsPlugins interface{}) ([]ap
if value, ok := response["external"]; ok {
result.External = value.(bool)
}
if value, ok := response["sideEffectFree"]; ok {
result.SideEffectFree = value.(bool)
}
if value, ok := response["pluginData"]; ok {
result.PluginData = value.(int)
}
Expand Down
14 changes: 11 additions & 3 deletions internal/bundler/bundler.go
Expand Up @@ -734,10 +734,18 @@ func runOnResolvePlugins(
return nil, true, resolver.DebugMeta{}
}

var sideEffectsData *resolver.SideEffectsData
if result.SideEffectFree {
sideEffectsData = &resolver.SideEffectsData{
IsSideEffectsArrayInJSON: false,
}
}

return &resolver.ResolveResult{
PathPair: resolver.PathPair{Primary: result.Path},
IsExternal: result.External,
PluginData: result.PluginData,
PathPair: resolver.PathPair{Primary: result.Path},
IsExternal: result.External,
PluginData: result.PluginData,
PrimarySideEffectsData: sideEffectsData,
}, false, resolver.DebugMeta{}
}
}
Expand Down
77 changes: 77 additions & 0 deletions internal/bundler/bundler_dce_test.go
@@ -1,9 +1,11 @@
package bundler

import (
"regexp"
"testing"

"github.com/evanw/esbuild/internal/config"
"github.com/evanw/esbuild/internal/logger"
)

var dce_suite = suite{
Expand Down Expand Up @@ -1655,3 +1657,78 @@ func TestTreeShakingInESMWrapper(t *testing.T) {
},
})
}

func TestPackageJsonSideEffectsFalsePluginResolver(t *testing.T) {
pk2Index := `
export {default as Cmp1} from './cmp1.vue';
export {default as Cmp2} from './cmp2';
`

testPackageJsonSideEffectsFalsePluginResolver(t, pk2Index)
}

func TestPackageJsonSideEffectsFalseNoPlugins(t *testing.T) {
pk2Index := `
export {default as Cmp1} from './cmp1';
export {default as Cmp2} from './cmp2';
`

testPackageJsonSideEffectsFalsePluginResolver(t, pk2Index)
}

func testPackageJsonSideEffectsFalsePluginResolver(t *testing.T, pkg2Index string) {
t.Helper()

mockFiles := map[string]string{
"/Users/user/project/src/entry.js": `
import {Cmp2} from "demo-pkg2"
console.log(Cmp2);
`,
"/Users/user/project/node_modules/demo-pkg2/cmp1.js": `
import {__decorate} from './helper';
let Something = {}
__decorate(Something);
export default Something;
`,
"/Users/user/project/node_modules/demo-pkg2/cmp2.js": `
import {__decorate} from './helper';
class Something2 {}
__decorate(Something2);
export default Something2;
`,
"/Users/user/project/node_modules/demo-pkg2/helper.js": `
export function __decorate(s) {
}
`,
"/Users/user/project/node_modules/demo-pkg2/package.json": `
{
"sideEffects": false
}
`,
"/Users/user/project/node_modules/demo-pkg2/index.js": pkg2Index,
}

dce_suite.expectBundled(t, bundled{
files: mockFiles,
entryPaths: []string{"/Users/user/project/src/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputFile: "/out.js",
Plugins: []config.Plugin{
Copy link
Owner

Choose a reason for hiding this comment

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

I'm going to remove this Go plugin test. There aren't really any Go-based plugin tests at the moment because they are essentially already covered by the end-to-end JavaScript plugin tests, since the API calls in each JavaScript plugin are forwarded through a Go plugin. Writing the tests in both JS and Go is duplicated effort.

{
OnResolve: []config.OnResolve{
{
Filter: regexp.MustCompile("\\.vue$"),
Callback: func(ora config.OnResolveArgs) config.OnResolveResult {
return config.OnResolveResult{
Path: logger.Path{Text: "/Users/user/project/node_modules/demo-pkg2/cmp1.js"},
SideEffectFree: true,
}
},
},
},
},
},
},
})
}
32 changes: 32 additions & 0 deletions internal/bundler/snapshots/snapshots_dce.txt
Expand Up @@ -414,6 +414,22 @@ console.log("hello");
// Users/user/project/src/entry.js
console.log(demo_pkg_exports);

================================================================================
TestPackageJsonSideEffectsFalseNoPlugins
---------- /out.js ----------
// Users/user/project/node_modules/demo-pkg2/helper.js
function __decorate(s) {
}

// Users/user/project/node_modules/demo-pkg2/cmp2.js
var Something2 = class {
};
__decorate(Something2);
var cmp2_default = Something2;

// Users/user/project/src/entry.js
console.log(cmp2_default);

================================================================================
TestPackageJsonSideEffectsFalseNoWarningInNodeModulesIssue999
---------- /out.js ----------
Expand Down Expand Up @@ -462,6 +478,22 @@ var init_a = __esm({
// Users/user/project/src/entry.js
Promise.resolve().then(() => (init_a(), a_exports)).then((x) => assert(x.foo === "foo"));

================================================================================
TestPackageJsonSideEffectsFalsePluginResolver
---------- /out.js ----------
// Users/user/project/node_modules/demo-pkg2/helper.js
function __decorate(s) {
}

// Users/user/project/node_modules/demo-pkg2/cmp2.js
var Something2 = class {
};
__decorate(Something2);
var cmp2_default = Something2;

// Users/user/project/src/entry.js
console.log(cmp2_default);

================================================================================
TestPackageJsonSideEffectsFalseRemoveBareImportCommonJS
---------- /out.js ----------
Expand Down
7 changes: 4 additions & 3 deletions internal/config/config.go
Expand Up @@ -490,9 +490,10 @@ type OnResolveArgs struct {
type OnResolveResult struct {
PluginName string

Path logger.Path
External bool
PluginData interface{}
Path logger.Path
External bool
SideEffectFree bool
PluginData interface{}

Msgs []logger.Msg
ThrownError error
Expand Down
2 changes: 2 additions & 0 deletions lib/shared/common.ts
Expand Up @@ -780,6 +780,7 @@ export function createChannel(streamIn: StreamIn): StreamOut {
let path = getFlag(result, keys, 'path', mustBeString);
let namespace = getFlag(result, keys, 'namespace', mustBeString);
let external = getFlag(result, keys, 'external', mustBeBoolean);
let sideEffectFree = getFlag(result, keys, 'sideEffectFree', mustBeBoolean);
let pluginData = getFlag(result, keys, 'pluginData', canBeAnything);
let errors = getFlag(result, keys, 'errors', mustBeArray);
let warnings = getFlag(result, keys, 'warnings', mustBeArray);
Expand All @@ -792,6 +793,7 @@ export function createChannel(streamIn: StreamIn): StreamOut {
if (path != null) response.path = path;
if (namespace != null) response.namespace = namespace;
if (external != null) response.external = external;
if (sideEffectFree != null) response.sideEffectFree = sideEffectFree;
if (pluginData != null) response.pluginData = stash.store(pluginData);
if (errors != null) response.errors = sanitizeMessages(errors, 'errors', stash, name);
if (warnings != null) response.warnings = sanitizeMessages(warnings, 'warnings', stash, name);
Expand Down
1 change: 1 addition & 0 deletions lib/shared/stdio_protocol.ts
Expand Up @@ -157,6 +157,7 @@ export interface OnResolveResponse {

path?: string;
external?: boolean;
sideEffectFree?: boolean;
namespace?: string;
pluginData?: number;

Expand Down
1 change: 1 addition & 0 deletions lib/shared/types.ts
Expand Up @@ -241,6 +241,7 @@ export interface OnResolveResult {

path?: string;
external?: boolean;
sideEffectFree?: boolean;
Copy link
Owner

Choose a reason for hiding this comment

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

I'm going to go with sideEffects: false instead of sideEffectFree: true to match Webpack. I think this will make the feature more intuitive.

namespace?: string;
pluginData?: any;

Expand Down
9 changes: 5 additions & 4 deletions pkg/api/api.go
Expand Up @@ -464,10 +464,11 @@ type OnResolveResult struct {
Errors []Message
Warnings []Message

Path string
External bool
Namespace string
PluginData interface{}
Path string
External bool
SideEffectFree bool
Namespace string
PluginData interface{}

WatchFiles []string
WatchDirs []string
Expand Down
1 change: 1 addition & 0 deletions pkg/api/api_impl.go
Expand Up @@ -1460,6 +1460,7 @@ func (impl *pluginImpl) OnResolve(options OnResolveOptions, callback func(OnReso

result.Path = logger.Path{Text: response.Path, Namespace: response.Namespace}
result.External = response.External
result.SideEffectFree = response.SideEffectFree
result.PluginData = response.PluginData

// Convert log messages
Expand Down
55 changes: 55 additions & 0 deletions scripts/plugin-tests.js
Expand Up @@ -709,6 +709,61 @@ let pluginTests = {
assert.strictEqual(result.default, 123)
},

async resolveWithSideEffectFree({ esbuild, testDir }) {
const input = path.join(testDir, 'in.js')
const cmp1 = path.join(testDir, 'cmp1.js')
const cmp2 = path.join(testDir, 'cmp2.js')
const cmpIndex = path.join(testDir, 'cmpIndex.js')
const helper = path.join(testDir, 'helper.js')

await writeFileAsync(input, `
import {Cmp2} from "./cmpIndex"
console.log(Cmp2);
`)
await writeFileAsync(cmp1, `
import {__decorate} from './helper';
let Something = {}
__decorate(Something);
export default Something;
`)
await writeFileAsync(cmp2, `
import {__decorate} from './helper';
let Something2 = {}
__decorate(Something2);
export default Something2;
`)
await writeFileAsync(cmpIndex, `
export {default as Cmp1} from './cmp1.vue';
export {default as Cmp2} from './cmp2';
`)
await writeFileAsync(helper, `
export function __decorate(s) {
}
`)

const result = await esbuild.build({
entryPoints: [input],
bundle: true,
write: false,
format: 'cjs',
plugins: [{
name: 'name',
setup(build) {
build.onResolve({ filter: /\.vue$/ }, async (args) => {
return {
path: path.join(args.resolveDir, args.path.replace('.vue', '.js')),
sideEffectFree: true,
};
});
},
}],
})

const output = result.outputFiles[0].text;

assert.doesNotMatch(output, /cmp1.js/);
},

async noResolveDirInFileModule({ esbuild, testDir }) {
const input = path.join(testDir, 'in.js')
const output = path.join(testDir, 'out.js')
Expand Down