Add error for dual packaging mistake #139
Conversation
lib/resolution/ResolutionContext.ts
Outdated
| if (!fs.existsSync(result)) { | ||
| throw new Error(`Could not dereference path ${result}. | ||
| In case the listed package is not a Node.js type, you might have an issue with dual packaging: | ||
| Your export should list your package.json explicitly.`); |
There was a problem hiding this comment.
I would consider adding an URL to the Node.js docs showing what they should do exactly.
There was a problem hiding this comment.
| return rootFile.replace(/index\.d\.ts$/u, `${packageName}.d.ts`); | ||
| const result = rootFile.replace(/index\.d\.ts$/u, `${packageName}.d.ts`); | ||
| // eslint-disable-next-line no-sync | ||
| if (!fs.existsSync(result)) { |
There was a problem hiding this comment.
Currently, resolvePackageIndex is a fast sync method, but this can make it quite slow.
Can you investigate if there is another way forward?
There was a problem hiding this comment.
Since the method also calls require.resolve, I doubt we pay a big additional cost?
There was a problem hiding this comment.
If you are still worried about this, I can look at adding some cache (since this code is only triggered for node modules, the cache size should not be to big). - I am not committed to finding a vastly different approach sorry.
There was a problem hiding this comment.
require.resolve works with a cache.
But if you think the performance impact will be insignificant, feel free to do some benchmarking.
|
@rubensworks I noticed for the unit tests you test using |
Could be a mistake indeed. It's been a long time since I've touched this code. |
| @@ -130,6 +130,11 @@ describe('ResolutionContext', () => { | |||
| expect(resolutionContext.resolvePackageIndex('stream', joinFilePath(__dirname, '../../'))) | |||
| .toEqual(joinFilePath(__dirname, '../../node_modules/@types/node/stream.d.ts')); | |||
| }); | |||
|
|
|||
| it('Should warn users when packages do not export their package.json', async() => { | |||
| expect(() => resolutionContext.resolvePackageIndex('@traqula/core', joinFilePath(__dirname, '../../'))) | |||
There was a problem hiding this comment.
Could we do this test with some basic mocking instead?
Adding traqula as dep seems like a lot of overhead for just a single test.
No description provided.