Improve tree shakability

[fisker]

Ref #608

As I tested, we only need change one place to fix it.

>yarn generate-esm && echo export{convertChangesToDMP}from"./libesm/index.js" | npx esbuild --bundle --format=esm
yarn run v1.22.19
$ yarn tsc --module nodenext --outDir libesm && echo {"type": "module","sideEffects":false} > libesm/package.json
$ E:\oss\forks\jsdiff\node_modules\.bin\tsc --module nodenext --outDir libesm
Done in 2.77s.
// libesm/convert/dmp.js
function convertChangesToDMP(changes) {
  var ret = [];
  var change, operation;
  for (var i = 0; i < changes.length; i++) {
    change = changes[i];
    if (change.added) {
      operation = 1;
    } else if (change.removed) {
      operation = -1;
    } else {
      operation = 0;
    }
    ret.push([operation, change.value]);
  }
  return ret;
}
export {
  convertChangesToDMP
};

[fisker]

Removed single quotes since it doesn't work on Windows.

[fisker]

Make sense also to add --target es6? Class should not be transformed in ESM.

[fisker]

I'm not sure if TypeScript allows use .mjs as extension, better use .mjs and remove the package.json.

[ExplodingCabbage]

I'm not sure if TypeScript allows use .mjs as extension, better use .mjs and remove the package.json.

I think it always matches the extension of the files being compiled (so the only way to get .mjs output is to name your source files .mts). At least, I haven't found any other way to tweak the extensions that tsc outputs.

Honestly, I found setting up a TypeScript build pipeline that was compatible with everything and made arethetypeswrong happy to be a total nightmare, but I see that at least some popular TypeScript libraries like Inquirer.js use the same pattern I settled on for jsdiff (having a CJS folder and an ESM folder, using .js extensions in both, having a package.json in each, and duplicating the type declarations across the two of them) so I think what I'm doing here must be idiomatic and non-awful!

[ExplodingCabbage]

Removed single quotes since it doesn't work on Windows.

Huh, interesting - do you personally develop on Windows? It looks to me like your changes here break this for non-Windows machines, since on my machine a package.json command like "generate-cjs": "echo {\"type\": \"commonjs\"} > file actually just writes {type: commonjs} to file (without the double quotes). I don't know CMD syntax well enough to know off the top of my head how to straightforwardly write this as a sh/CMD polyglot.

Are there any widely-accepted norms about how to go about supporting Windows in package.json scripts? It seems to me it'd be simpler to only support one species of shell language; perhaps I should put something in the README advising Windows people to use Git-Bash or WSL? Or I could wrap all shell commands in a sh -c invocation to ensure that, if they're executed at all, it's by sh and not CMD (but that's still only partly useful to Windows users unless they've got sh on their PATH).

[ExplodingCabbage]

Make sense also to add --target es6? Class should not be transformed in ESM.

Hmm. Does it do any harm to transform them? They can still be instantiated and can still be extended with extends; is there anything that gets broken by the transformation?

[fisker]

No to all your questions, but it's unnecessary, since ESM version can't run in es5 environment, but it's up to you.

[fisker]

Are there any widely-accepted norms about how to go about supporting Windows in package.json scripts?

Prettier uses node --eval "fs.writeFileSync()", this line was changed many times, funny story.

[fisker]

2425402 (#612)

[fisker]

No harm to have this in the root package.json.

[ExplodingCabbage]

Hmm. dist/diff.js and dist/diff.min.js do have side effects - they create the Diff global. Probably this doesn't matter; I think there's no way anybody could be consuming those files that would respect sideEffects: false, given that they're not even exported via our exports property in package.json. But I am not 100% sure of that. If it's not serving a purpose, I am inclined not to add this line here, just in case. (Leaving out the one that doesn't do anything also makes it clear which property is actually having an effect, if we want to change it later.)

[ExplodingCabbage]

I am not sure if it'll serve a purpose, but I figure we should add "sideEffects": false here too? It's true and could plausibly be useful, since a quick google reveals that tree-shaking plugins for CommonJS do exist. (I have no idea whether they respect "sideEffects": false, but even if they don't today, they might do tomorrow.)

[fisker]

I didn't add it since Prettier only use ESM version 😄

[ExplodingCabbage]

Thank you!

[fisker]

Just want let you know, Prettier is able to save ~5K and can use import {} from "diff" directly. prettier/prettier#17506

👍