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

Review Request #1

Open
antfu opened this issue Jul 4, 2023 · 9 comments
Open

Review Request #1

antfu opened this issue Jul 4, 2023 · 9 comments

Comments

@antfu
Copy link
Member

antfu commented Jul 4, 2023

Hi all,

We have been supporting #__PURE__ for a long time but I think there is lacking a proper documentation/specification for it. If you Google it, the top result is a StackOverflow answer that points to a very long webpack docs with a lot of other concepts involved.

So, here I want to kickstart the specs for the #__PURE__ notation as well as the other notations we support or going to support. For easier reference and documentation.

I have written some very basic drafts here and seeking collaborations and contributions to improving them together.

Will shall find a better org name for sure. If you are a maintainer of bundlers/minifiers, let me know if you are interested in joining the org :)

/cc @lukastaegert @skyrpex @Andarist @alexlamsl @evanw @kdy1 @TheLarkInn @fabiosantoscode
(sorry for pinging, feel free to unsubscribe)

Any feedback is greatly welcome, thank you!

@kdy1
Copy link
Contributor

kdy1 commented Jul 5, 2023

I love the idea 😄

@fabiosantoscode
Copy link

I can see the usefulness in documenting these in great specificity so more tools can make use of them. Currently the spec for PURE and such is spread out in issue comments and the source code of the tools (as far as I know).

But I don't think changes to annotations are going to happen.

@antfu
Copy link
Member Author

antfu commented Jul 6, 2023

@fabiosantoscode Yeah I think the goal is here not to force every tool to align, but more like documenting that are already working.

@hyrious
Copy link

hyrious commented Jul 7, 2023

The #__PURE__ Notation Spec does not all apply to esbuild. Becasue of the main goal (fast) of esbuild, it does not do many analysis and its rules are the most pedantic one between bundlers.

I have some tests before, the minimal spec of PURE in esbuild can roughly be:

Syntax
The #__PURE__ notation marks the right most calling of the next expression as having no side effects. Examples:
  • let a = /*#__PURE__*/ Promise.resolve()
    The whole expression is pure.
  • let a = /*#__PURE__*/ f.g().h().i
    The f.g().h() is pure, but it is then being used to access .i, which causes the whole expression not pure.
  • let a = /*#__PURE__*/ f()()
    The whole expression is pure.
  • let a = /*#__PURE__*/ new TextEncoder()
    The whole expression is pure.
  • let a = /*#__PURE__*/ (() => { many.sideEffect.here() })()
    The whole expression is pure.
Wrong examples:
  • let a = /*#__PURE__*/ f.g.h.i
    Accessing properties is not calling and is always not pure.
  • /*#__PURE__*/ let a = f.g().h().i
    Nothing is marked pure here.
  • /*#__PURE__*/ function f() {}
    Functions are pure by default, do not need this annotation.

Note that pure annotation is not the only way that can control the tree shaking process. In esbuild the "sideEffects" field in package.json also takes effect. For example, you can mark the whole folder as pure (to be shaked) if no imports from them are being used. Example:

// package/some-folder/package.json
{ "sideEffects": false }

// package/test.js
import { foo } from './some-folder/some-file' // this line will be tree-shaked

@mizchi
Copy link

mizchi commented Jul 10, 2023

Does this spec also include processing related to mangling and inline expansion (mainly terser does that)?

https://github.com/terser/terser#annotations

According to this document, there are __INLINE__, __NOINLINE__, __KEY__, __MANGLE_PROP__, and __PURE__. I think the __PURE__ in rollup comes from terser(or uglifyjs).

Also a bit special is TypeScript's @internal comment, which when used with compilerOptions.stripInternal: true omits the property type definition from .d.ts. If you refer to the type definition when mangling, this may be the final control involved.

https://www.typescriptlang.org/tsconfig#stripInternal

Of course, these are advanced things that are some distance from bundle, and their feasibility depends on the compiler design. So I don't think all bundlers should have this feature, but having a reference may increase interoperability.

My context

I am experimenting with a minifier using TypeScript's LanguageService. The goal of this repository is to do type-level mangling and DCE, which is not possible with terser. (DCE has not been started yet).

https://github.com/mizchi/packelyze/tree/main/transformer

Example of working mangling: https://github.com/mizchi/packelyze/blob/main/transformer/src/transformer/mangler.test.ts#L472-L542

As per the motivation of this spec, documentation on __NO_SIDE_EFFECT__ was lacking, so this is a great help. Thanks!

@antfu
Copy link
Member Author

antfu commented Jul 10, 2023

@mizchi AFAIK, __PURE__ is the most common notation that is supported in all major bundlers. For the rest, I think it would also be reasonable to have them if the Terser team wants to push it more to a common standard.

For @internal I personally think would be a bit out-of-scope. But maybe it can be a good idea that we reference most of the state-of-art, as you mentioned, and also:

@mizchi
Copy link

mizchi commented Jul 10, 2023

@antfu
The optimization with magic comments in terser is more about general JavaScirpt optimization than bundle handling, so it would certainly be better to have it outside of this spec. Since __PURE__ is the basic functionality, I agree that the spec should be built around it.

In that light, I guess you could say that what this repository deals with is the spec for combining multiple files that cannot be achieved in a single file.

@lukastaegert
Copy link
Contributor

The __PURE__ spec currently does not reflect how it works in Rollup, and to my understanding UglifyJS and Terser. Here, these annotations ONLY apply to call expressions and new expressions and not (as the current spec suggests) function declarations. It can only be placed directly preceding the call expression.
For instance the logic in Rollup is:

  • If anything but a comment or white-space separates the annotation from the call expression, it is considered invalid and removed.
  • An exception or parentheses. If parentheses surround a single call or new expression, then an annotation placed before them is applied to the call.
    • Good example: /*#__PURE__*/ (foo());
    • Here it does not apply: /*#__PURE__*/ (foo(), bar()); The reason is, that technically, the annotation precedes foo(), but semantically, the comma expression evaluates to bar(). To avoid this ambiguity, the annotation is considered invalid here.
  • In a case like /*#__PURE__*/ a.b().c.d(), a.b().c.d is considered the "callee" and the annotation is applied to the last call (as @hyrious correctly stated for esbuild, which to my understanding based their behavior on Rollup's).
  • If a call is annotated, Rollup does not search for further potential side effects in the callee like invalid property accesses.
  • In an example like /*#__PURE__*/ a().b, the outermost expression is a property access of b. In that situation, the annotation is considered invalid.

Would you be fine with changing the documentation in such a way?

@antfu
Copy link
Member Author

antfu commented Aug 15, 2023

Oh yeah, thanks for pointing out @lukastaegert! I guess I was mistakenly mixed the code for no-side-effect. If you'd like, it would be great if you could help update directly with a PR :)

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

6 participants