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 #__NO_SIDE_EFFECTS__ comment from Rollup #3149

Closed
antfu opened this issue Jun 7, 2023 · 6 comments
Closed

Support #__NO_SIDE_EFFECTS__ comment from Rollup #3149

antfu opened this issue Jun 7, 2023 · 6 comments

Comments

@antfu
Copy link

antfu commented Jun 7, 2023

With a similar reasoning:

We recently shipped #__NO_SIDE_EFFECTS__ in Rollup (rollup/rollup#5024). We wish to push this convention to be supported in other build tools and to benefit library authors to make more portable and flexible libraries.

Would love the hear the thoughts from esbuild side. Thanks!

@antfu
Copy link
Author

antfu commented Jun 7, 2023

Currently, when using rollup-plugin-esbuild, the comments are not preserved, and __NO_SIDE_EFFECTS__ is not recognized by esbuild. We need to use /*! to preserve them.

It would be great to esbuild been able to preserve it, or even provide tree-shaking capability with that convention.

@antfu antfu changed the title Support #__NO_SIDE_EFFECTS__ from Rollup Support #__NO_SIDE_EFFECTS__ comment from Rollup Jun 7, 2023
@paulmillr
Copy link

Why is it #__NO_SIDE_EFFECTS__, but /* @__PURE__ */? Does @__NO_SIDE_EFFECTS__ also work?

@zaygraveyard
Copy link

@paulmillr I believe both #__NO_SIDE_EFFECTS__ and @__NO_SIDE_EFFECTS__ work, just like both #__PURE__ and @__PURE__ work.

@evanw
Copy link
Owner

evanw commented Jun 9, 2023

I can look into preserving comments containing /* #__NO_SIDE_EFFECTS__ */. I don't think it's possible to actually implement this feature in esbuild with esbuild's current tree shaking code as side effect detection is currently done locally for each file.

@evanw
Copy link
Owner

evanw commented Jun 10, 2023

I looked into this. A few questions:

  • Is there any documentation about how this is supposed to work? I found this but it isn't a specification (e.g. it doesn't say exactly where you can use these comments). So I'm trying to reverse-engineer the expected behavior from the Rollup REPL.

  • It looks like the annotation sort of works with variable declarations, but only for the first declaration and only for const. Is that right?

  • Is there a reason why this annotation doesn't work with async functions? It seems to work with regular functions and generator functions.

  • It doesn't appear to work for object methods, class methods, or class fields. So it shouldn't work in these places when using esbuild either then, right?

Edit: I'm going with copying Rollup's current observable behavior (at least as far as I can tell). So to be clear, the annotation can now be used in all of these positions:

/* @__NO_SIDE_EFFECTS__ */ function f() {}
/* @__NO_SIDE_EFFECTS__ */ function* f() {}
/* @__NO_SIDE_EFFECTS__ */ export function f() {}
/* @__NO_SIDE_EFFECTS__ */ export function* f() {}

/* @__NO_SIDE_EFFECTS__ */ export default function f() {}
/* @__NO_SIDE_EFFECTS__ */ export default function* f() {}
export default /* @__NO_SIDE_EFFECTS__ */ function f() {}
export default /* @__NO_SIDE_EFFECTS__ */ function* f() {}

const f = /* @__NO_SIDE_EFFECTS__ */ function() {}
const f = /* @__NO_SIDE_EFFECTS__ */ function*() {}
export const f = /* @__NO_SIDE_EFFECTS__ */ function() {}
export const f = /* @__NO_SIDE_EFFECTS__ */ function*() {}
/* @__NO_SIDE_EFFECTS__ */ const f = function() {}
/* @__NO_SIDE_EFFECTS__ */ const f = function*() {}
/* @__NO_SIDE_EFFECTS__ */ export const f = function() {}
/* @__NO_SIDE_EFFECTS__ */ export const f = function*() {}

const f = /* @__NO_SIDE_EFFECTS__ */ () => {}
export const f = /* @__NO_SIDE_EFFECTS__ */ () => {}
/* @__NO_SIDE_EFFECTS__ */ const f = () => {}
/* @__NO_SIDE_EFFECTS__ */ export const f = () => {}

class Foo { static f = /* @__NO_SIDE_EFFECTS__ */ function() {} }
class Foo { static f = /* @__NO_SIDE_EFFECTS__ */ function*() {} }
class Foo { static f = /* @__NO_SIDE_EFFECTS__ */ () => {} }
x = { f: /* @__NO_SIDE_EFFECTS__ */ function() {} }
x = { f: /* @__NO_SIDE_EFFECTS__ */ function*() {} }
x = { f: /* @__NO_SIDE_EFFECTS__ */ () => {} }

And the annotation cannot be used in any of these positions (not an exhaustive list, but perhaps still worth listing for clarity):

/* @__NO_SIDE_EFFECTS__ */ async function f() {}
/* @__NO_SIDE_EFFECTS__ */ async function* f() {}
/* @__NO_SIDE_EFFECTS__ */ export async function f() {}
/* @__NO_SIDE_EFFECTS__ */ export async function* f() {}

/* @__NO_SIDE_EFFECTS__ */ export default async function f() {}
/* @__NO_SIDE_EFFECTS__ */ export default async function* f() {}
export default /* @__NO_SIDE_EFFECTS__ */ async function f() {}
export default /* @__NO_SIDE_EFFECTS__ */ async function* f() {}

const f = /* @__NO_SIDE_EFFECTS__ */ async function() {}
const f = /* @__NO_SIDE_EFFECTS__ */ async function*() {}
export const f = /* @__NO_SIDE_EFFECTS__ */ async function() {}
export const f = /* @__NO_SIDE_EFFECTS__ */ async function*() {}
/* @__NO_SIDE_EFFECTS__ */ const f = async function() {}
/* @__NO_SIDE_EFFECTS__ */ const f = async function*() {}
/* @__NO_SIDE_EFFECTS__ */ export const f = async function() {}
/* @__NO_SIDE_EFFECTS__ */ export const f = async function*() {}

const f = /* @__NO_SIDE_EFFECTS__ */ async () => {}
export const f = /* @__NO_SIDE_EFFECTS__ */ async () => {}
/* @__NO_SIDE_EFFECTS__ */ const f = async () => {}
/* @__NO_SIDE_EFFECTS__ */ export const f = async () => {}

class Foo { /* @__NO_SIDE_EFFECTS__ */ static f = function() {} }
class Foo { /* @__NO_SIDE_EFFECTS__ */ static f = function*() {} }
class Foo { /* @__NO_SIDE_EFFECTS__ */ static f = () => {} }
class Foo { /* @__NO_SIDE_EFFECTS__ */ static f() {} }
class Foo { /* @__NO_SIDE_EFFECTS__ */ static *f() {} }
class Foo { /* @__NO_SIDE_EFFECTS__ */ static get f() {} }
x = { /* @__NO_SIDE_EFFECTS__ */ f: function() {} }
x = { /* @__NO_SIDE_EFFECTS__ */ f: function*() {} }
x = { /* @__NO_SIDE_EFFECTS__ */ f: () => {} }
x = { /* @__NO_SIDE_EFFECTS__ */ f() {} }
x = { /* @__NO_SIDE_EFFECTS__ */ *f() {} }
x = { /* @__NO_SIDE_EFFECTS__ */ get f() {} }

@evanw evanw closed this as completed in cf687c9 Jun 10, 2023
@antfu
Copy link
Author

antfu commented Jun 11, 2023

Thanks a lot for the prompt support!

We were discussing with @lukastaegert that we should have a repo hosting the spec for __PURE__ as well as __NO_SIDE_EFFECTS__, as where were never a proper doc/spec for them. We will see!

As for the async function and generator, that's a good point and actually was my mistake to miss them. I will send a PR to fix it on Rollup soon. I guess that makes sense for esbuild to still present the comments for them as well.

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

No branches or pull requests

4 participants