search

RTVision / esbuild-dynamic-import

Plugin for transform dynamic imports in esbuild
17 stars 5 forks source link

Incorrect Docs? #3

Closed shellscape closed 1 year ago

shellscape commented 1 year ago

Hey there, thanks for the plugin. Looks like your docs might be incorrect:

// note depending on your setup you may need to do DynamicImport.default() instead

DynamicImport doesn't have a default property.

kalvenschraut commented 1 year ago

Depends on enviroment (node, bun, deno, some bundler) and module system you are using commonjs vs ES modules

If there is no default property then DefaultImport is probably a function itself and you just call that.

shellscape commented 1 year ago

That's a pretty lazy take on a README, especially if you leave it up to the user to derive that nuance on their own.

kalvenschraut commented 1 year ago

Could the README try explain that nuance? Maybe, but it is a pretty common one and not unique to this repo at all.

I would expect most users to notice, as you did, the default property doesn't exist so they must have to just call DynamicImport directly as code example is actually doing.

CommonJS vs ESM incompatibilities are definitely one of the bigger pains to try deal with ATM until everyone transitions to ESM as that seems to be the direction.

kalvenschraut commented 1 year ago

Actually did just notice that this repo is still telling typescript to compile down to CommonJS... would have to double check, but compiling straight to ESM may alleviate the need for that comment.

shellscape commented 1 year ago

If we did that kind of thing in the docs with Rollup at 7mil downloads a week, we'd have users with pitchforks at our doors. Be a good steward, don't burden users.

kalvenschraut commented 1 year ago

Swapped to tsup and providing CJS and ESM builds with export mapping with fallback to main CJS build in 1.0.1.

I believe that fixes the need for that comment.