fix: disable esModuleInterop
#375
Conversation
test/abort-task.ts
Outdated
| @@ -1,6 +1,6 @@ | |||
| import { AbortController } from 'abort-controller'; | |||
| import { EventEmitter } from 'events'; | |||
| import Piscina from '..'; | |||
| import Piscina = require('..'); | |||
There was a problem hiding this comment.
Let's tackle this differently, by exporting Piscina in different manners by exporting it as default, and named export.
Otherwise, this can potentially break the current user's expectations if esModuleInterop is set to false. Wdyt?
There was a problem hiding this comment.
does the default export created by TS work as an actual default export in Node? The current one does as it's just module.exports =
There was a problem hiding this comment.
If exported by default, it will export it as module.exports.default = Piscina, if by named export, it translates to module.Piscina = Piscina. If just used as export Piscina, it just does module.exports = Piscina.
Exporting it as default, and named will enable imports like:
import { Piscina } from 'piscina'import Piscina, { SomeType } from 'piscina'
There was a problem hiding this comment.
seems #517 did this 👍
src/index.ts
Outdated
| import { AsyncResource } from 'async_hooks'; | ||
| import { cpus } from 'os'; | ||
| import { fileURLToPath, URL } from 'url'; | ||
| import { resolve } from 'path'; | ||
| import { inspect, types } from 'util'; | ||
| import assert from 'assert'; | ||
| import assert = require('assert'); |
There was a problem hiding this comment.
Maybe we can change the way this is imported as discussed in #374 ?
test/abort-task.ts
Outdated
| @@ -1,6 +1,6 @@ | |||
| import { AbortController } from 'abort-controller'; | |||
| import { EventEmitter } from 'events'; | |||
| import Piscina from '..'; | |||
| import Piscina = require('..'); | |||
There was a problem hiding this comment.
If exported by default, it will export it as module.exports.default = Piscina, if by named export, it translates to module.Piscina = Piscina. If just used as export Piscina, it just does module.exports = Piscina.
Exporting it as default, and named will enable imports like:
import { Piscina } from 'piscina'import Piscina, { SomeType } from 'piscina'
|
This issue has been marked as stale because it has been opened 45 days without activity. Remove stale label or comment or this will be closed in 10 days. |
|
Completely forgot about this - can take a look at reviving tomorrow |
SimenB
force-pushed
the
disable-esModuleInterop
branch
from
abba498
to
39acbe3
Compare
|
@metcoder95 due to #517, this PR simply needs to flip the flag and fix the |
| @@ -5,7 +5,7 @@ import { availableParallelism } from 'os'; | |||
| import { fileURLToPath, URL } from 'url'; | |||
| import { resolve } from 'path'; | |||
| import { inspect, types } from 'util'; | |||
| import assert from 'assert'; | |||
| import { strict as assert } from 'assert'; | |||
There was a problem hiding this comment.
I doubt the change to use strict is visible to consumers, but assert is used as a function, so we need to tweak this import
|
Sorry, finally made the tests works as expected; overall it seems there's an issue with the exports 🤔 |
|
Oh, interesting! I'll take a new look tomorrow 😅 |
|
Right, the problem is that the types file do not reflect what is actually used at runtime by TS - types point to |
|
I've been thinking about refactoring the way the types are overall exported, as currently everything happens with the If you are willing to, I'm more than happy support you on a PR that refactors the way types are exported. Meanwhile we are able to do const Piscina = require('piscina');
// or
const { Piscina } = require('piscina');
// and ofc
import { Piscina, HelperPiscinaTypes } from 'piscina';
import Piscina, { HelperPiscinaMethods } from 'piscina'; |
|
This issue has been marked as stale because it has been opened 45 days without activity. Remove stale label or comment or this will be closed in 10 days. |
|
not stale, just low on OSS time 😅 It's on my list! |
|
This issue has been marked as stale because it has been opened 45 days without activity. Remove stale label or comment or this will be closed in 10 days. |
|
On vacation, will get back to this (hopefully in August) |
metcoder95
added
backport v4
|
This issue has been marked as stale because it has been opened 45 days without activity. Remove stale label or comment or this will be closed in 10 days. |
#374, but going as far as disabling the option in this repo to avoid regressions.
I'm having some issues getting tests to run with this