Skip to content

Conversation

@gustawdaniel
Copy link
Contributor

@gustawdaniel gustawdaniel commented Mar 26, 2023

#16

Js -> TS

To build we need:

  • ejs
  • cjs
  • src
  • package.json

Breaking changes:

@spence-s
Copy link
Owner

spence-s commented Mar 26, 2023

This is really great, I appreciate the refactor to TS.

However, I want to keep this library built for node.js as it currently is.

Please remove esbuild, and use tsc to create the build. Target Node16 as your output and emit d.ts files as well so type defs are included. Add anything to support this to package.json that is needed.

I'm fine with moving tests to jest, but please just run it with a ts-node loader.

Copy link
Owner

@spence-s spence-s left a comment

Choose a reason for hiding this comment

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

Remove esbuild - use tsc to target commonjs and Node16.

@spence-s
Copy link
Owner

Also, lets keep the default export - there is no reason to change that.

README.md Outdated
## Configuring

```typescript
(async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

this example provides no value. You can do this pattern in cjs or esm and there is no reason to show that.

cjs

const HR = require('human-object-diff')

esm

import HR from 'human-object-diff'

dynamic import in esm

const {default: HR} = await import("human-object-diff")

dynamic import in cjs

(async() =>{
  const {default: HR} = await import("human-object-diff")
})()

All of the above is supported by just exporting a single cjs build.

@gustawdaniel
Copy link
Contributor Author

@spence-s I will remove esm as you wish.

But please explain me, what advantage has tsc over esbuild?

For me esbuild is better because is few times faster. I am really curious about reason, why somebody can prefer tsc.

@spence-s
Copy link
Owner

@spence-s I will remove esm as you wish.

But please explain me, what advantage has tsc over esbuild?

For me esbuild is better because is few times faster. I am really curious about reason, why somebody can prefer tsc.

As I said - we are not removing ESM support. Anyone can still use this library in a pure ESM environment for both node/browser/anything with just a cjs build.

For advantages, it's more about simplicity and maintainability. ESBuild is a bundler -- the set up you have here takes all the dependencies in the library and all the files and transforms them into 1 file which do not import anything at all. All the dependencies are squished into this 1 file. Not only is this unneeded, it makes the output very obfuscated for no reasons. If there are type errors or problems in the code (these still happen even with TS), then Error traces will be cryptic.

I want users to be able to download the code, see it in their node_modules, and understand it if needed as much as possible and I want the output to reflect the code as it was originally written. tsc does the minimal required work.

ESBuild is really good sometimes, in some situations. I agree. I use it on a lot of different things. I don't dislike it. But I do dislike it for making library builds for NodeJS libraries. I could potentially support writing this package a pure ESM package and then using esbuild to re-write for cjs compatibility. That is NOT what this PR does. This pr sticks to writing the code as CJS code and then uses a bundler to make an ESM build which adds no real value to me. The pure CJS output, as stated above, can be used by ESM at any time.

Bundling is an opinionated process. That should be left up to users and gives them power to optimize their code. If we pr-bundle, we are forcing all of the deps we use here on them and if they have duplicate deps, they won't be able to drop them.

As library maintainers here, we want to give the users max insight and power and that is best achieved by doing the minimal amount of build steps and that is exactly what tsc does out of the box, without adding the extra esbuild build dependency. Also this library is small, and tsc and esbuild are both super fast. If this lib were HUGE, then you may have a good point about switching to esbuild, but its v fast with tsc already.

Hopefully that all makes sense to you. Happy to answer any questions and am open to having my mind changed if you have a really good argument for the setup here. However, "esbuild is faster" is not a good enough argument for me at all.

Repository owner deleted a comment from gustawdaniel Mar 27, 2023
@spence-s
Copy link
Owner

I would support a separate esm build under the following conditions:

In both builds:

  1. Each file get an output that is not bundled and the same dir and file structure is preserved in the outputs.
  2. tests are ran against both ESM and CJS builds, currently in the PR it is only ran against CJS.
  3. Proper declaration files are emitted in a way that works for both builds in Node.js
  4. The source code was refactored to pure ESM (ie... type: module in pacakge.json)

However, I think most of above doesn't add much extra value, since CJS supports everything out of the box with none of the above steps or problems.

@gustawdaniel
Copy link
Contributor Author

gustawdaniel commented Mar 27, 2023

I stuck with the following problem:

In index.ts i can write

class DiffEngine {}
module.exports = DiffEngine;

and then:

const HumanDiff = require('human-object-diff')

works but:

import HumanDiff from 'human-object-diff';

not

import * as  HumanDiff from 'human-object-diff';

or

const  HumanDiff = await import('human-object-diff');

is

console.log(HumanDiff);

and i have to write ts-ignore to use it

// @ts-ignore
const { diff } = new HumanDiff.default(options);


but on other hand when I write

class DiffEngine {};
export default DiffEngine;

then after

const HumanDiff = require('human-object-diff')

i have to break compatibility and use as

const { diff } = new HumanDiff.default(options);

but for

import HumanDiff from 'human-object-diff'; 
const  HumanDiff = await import('human-object-diff');

it works as before

@gustawdaniel
Copy link
Contributor Author

Fixed by

// CommonJS export
module.exports = DiffEngine;
module.exports.default = DiffEngine;

// ES module export
export default DiffEngine;

@spence-s
Copy link
Owner

spence-s commented Mar 27, 2023

You only need to do: export default DiffEngine in ts.

You do not need to use module.exports at all or module.exports.default

Note: CJS is a compile target from typescript, you still use import/export in the ts code.

Your compiled code will be module.exports = DiffEngine

@gustawdaniel gustawdaniel requested a review from spence-s March 27, 2023 15:35
@spence-s
Copy link
Owner

@gustawdaniel I think it will be easiest if I just merge this, fix the build, then we can get in your code changes/fixes!

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

Successfully merging this pull request may close these issues.

2 participants