fix: esModuleInterop not working for object-hash and dataloader imports

[turulix]

Fixes #793

[stephenh]

Hi @turulix , thanks for the PR! I've got a hopefully minor ask to try and let ts-poet do the rewriting/esModuleInterop conditionalization for us, but otherwise this looks great!

[stephenh]

Fwiw, this is admittedly kind of cute, but if you'd like how I've handled this in the past is by having ts-poet rewrite the imports.

That was more useful when the imports were scattered all over the ts-proto codebase, while granted for these imports, they're only used ~once/twice so it's not that bad.

That said, if you'd like to try it, I just pushed a new version of ts-poet to handle default --> star rewrites (stephenh/ts-poet@29c834d) so, in theory you could:

• Bump ts-poet in package.json to 6.4.1
• Keep these consts by as `const hash = imp("hash=object-hash");
• Update getTsPoetOpts to include dataloader and object-hash in the imports list, which should a) leave them as-is if esModuleInterop is on, or b) rewrite them to the old import ... as * if esModuleInterop is not on

Do you mind trying that and seeing if it works?

[turulix]

I will try it tomorrow or on Monday. Also, I noticed that the 'long' package is imported in the same way that I did in this pull request - I took the syntax from there. Should I do the same thing with it, as mentioned above?

[stephenh]

Ah yeah, if the long imports can take advantage of the new ts-poet capability, it'd be great to move those over too. I must have forgotten about those when I taught ts-poet how to handle the protobufjs esModuleInterop conditional imports. Thank you!

[turulix]

Bumping ts-poet to 6.4.1 lets the utils-test fail , cause toCodeString() now requires some arguments.

[stephenh]

Ah shoot...sorry about that; I think toCodeString was kind of an internal api.

Glancing at it locally, it looks like just changing all the toCodeString -> toString and then updating the inline snapshots fixes it.

Would you like me to push out/merge a dedicated "bump-ts-poet" branch, or do you want to incorporate this fix into your branch?

[turulix]

Changing it to toString() makes it fail in a different way. So probably just make that a different branch.
)

[stephenh]

@turulix cool, I just merged the ts-poet bump to main, so you should be able to rebase/merge on top of it. Thanks!

[turulix]

Not sure if i'm doing something wrong, but doing

  const imports = [
    "protobufjs/minimal" + _options.importSuffix,
    "object-hash" + _options.importSuffix,
    "dataloader" + _options.importSuffix
  ];

leaves them as import * as ...

[stephenh]

Ah yeah, you should be able to leave off the import suffix, so just like:

  const imports = [
    "protobufjs/minimal" + _options.importSuffix,
    "object-hash",
    "dataloader"
  ];

The import suffix is just for doing *.js / *.ts imports in strict ESM mode, but that shouldn't apply to packages like dataloader or object-hash.

...even protobufjs/minimal having the import suffix I'm a little curious about, but let's leave that as-is for now.

[turulix]

I assumed its just an empty String if its not set, cause never saw it generate unless i explicitly set it. So it shouldnt have any impact on it

[turulix]

It still dosn't change it when removing the suffix.

[stephenh]

🎉 This PR is included in version 1.148.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release
• v1.148.2

Your semantic-release bot 📦🚀