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

Design Proposal: Leverage `side-effects: false` in package.json for tree-shaking friendly modules #19202

Closed
donaldpipowitch opened this Issue Oct 16, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@donaldpipowitch
Contributor

donaldpipowitch commented Oct 16, 2017

Before You Start

I think this design proposal aligns with TypeScript Design Goals and helps TypeScript authors to write more efficient packages (and help the JavaScript ecosystem in general this way).

The are very similar existing suggestions, but it looks like none of them leverages side-effects: false in package.json directly.

E.g. there is

  • #17181: Flags parts of your code as pure.

Defining a Scenario

As a lib author I'd like to create packages which are easy to consume by tools and allow features like efficient tree shaking. I'd like to re-use existing conventions to flag my package with a certain feature (in this case side-effects: false in package.json). I'd like TypeScript to assist me in writing packages which fulfil this constraint.

So basically:

  • When I write a TypeScript-based package...
  • ...and set side-effects: false in package.json...
  • ...I want the TypeScript compiler to throw errors, when I invalidate this constraint.

Language Feature Checklist

This change should be able to be introduced without a breaking change and should keep backwards compatibility with the JS language.

Side effects free packages

See here for an example.


cc @TheLarkInn

@kitsonk

This comment has been minimized.

Contributor

kitsonk commented Oct 16, 2017

While there might be micro-libraries that can enforce that constraint, a lot of common code is side-effecty and a lot of side-effecty code isn't determinable at compile time, though CFA has advanced itself quite a lot over the past little while, I doubt it is capable of tracking that fully reliably at the moment.

@donaldpipowitch

This comment has been minimized.

Contributor

donaldpipowitch commented Oct 16, 2017

What is CFA? :)

Maybe the side effect must not be perfect in the beginning and can just become better in multiple steps (like our type system becomes more feature-rich as well over time). I think there can be multiple quick wins (e.g. keep globals and deps as read-only from outside of exported functions and so on).

@mhegazy

This comment has been minimized.

Contributor

mhegazy commented Oct 16, 2017

Why can not this be done today by a tslint rule/additional tool ?

@aluanhaddad

This comment has been minimized.

Contributor

aluanhaddad commented Oct 16, 2017

@donaldpipowitch I am strongly in favor of writing code that is free of side effects but I don't see what this practice has to do with metadata in a package.json manifest. Even if it is of use to tools that perform tree shaking optimizations I don't think that is the primary purpose of writing such code as it often trades performance for maintainability. Basically, if I was going to look for purity, I wouldn't think to look in a manifest, that just seems strange.

CFA stands for control flow analysis (definitely an acronym that needs to be looked up the first time but very useful when talking about it 😃)

@kitsonk

This comment has been minimized.

Contributor

kitsonk commented Oct 17, 2017

Basically, if I was going to look for purity, I wouldn't think to look in a manifest, that just seems strange.

Yes, and people like to throw definitions around liberally of what it means. Pure specifically means a) no side effects and b) no hidden state. People largely forget the second one, and the second one has no impact on if code is elidable or not, though it has a huge impact on testability.

I agree with Mohamed, in that if this were something that was closely aligned to the type system, then it might make sense for TypeScript, but it is ultimately an approach to structuring code which can lead to optimized code that is easier to maintain and easier to test, but it isn't the only approach. The ability to annotate such code is important, but the ability to enforce that pattern at a project level seems like a higher order concern for a linting or other tool. TypeScript only over imposes itself when it can identify a static error. Side effecty code is not an error, it is at worst an anti-pattern for some.

@aluanhaddad

This comment has been minimized.

Contributor

aluanhaddad commented Oct 17, 2017

@kitsonk indeed, and pure was a poor choice of words on my part, and to be fair it wasn't used in the original post.

Considering there is currently not a way to convey the absence of side effects to the typechecker this seems out of scope.

@donaldpipowitch

This comment has been minimized.

Contributor

donaldpipowitch commented Oct 17, 2017

Why can not this be done today by a tslint rule/additional tool ?

You're right, that would work. Maybe I'm biased here, because I personally would like to see a tighter integration between linters and TypeScript (see #15589 for this topic).

I don't see what this practice has to do with metadata in a package.json manifest.

I maintain a lot of build and tooling configurations and I really love it when multiple tools agree on a common way for certain settings so other tools can build upon that as well. (A nice example is browserslist which was used to configure a CSS prefixer and now also powers babel-preset-env. You basically specificy in your package.json which runtimes/browsers you officially support. E.g. I could imagine TypeScript to use this as well to maybe auto-configure things like --lib.)

I didn't want to say tree shaking optimizations are the primary reasons to write side effects free code, I just wanted to point out that webpack tries to establish a convention to flag packages a side effects free :)

CFA stands for control flow analysis

Ah! Thanks. Didn't think about control flow analysis here :)

@mhegazy

This comment has been minimized.

Contributor

mhegazy commented Oct 20, 2017

We still think of immutability as a general concept, see #19169 for more details. I think this request is rather specific and is achievable today without changing the compiler or the language. This issue is outside the scope of the project for the time being.

@donaldpipowitch

This comment has been minimized.

Contributor

donaldpipowitch commented Oct 20, 2017

Okay, Thank you. I guess it is better to close the issue for now.

@Microsoft Microsoft locked and limited conversation to collaborators Jun 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.