fix(node-resolve): Resolve local files if resolveOption is set

[JaviIG]

Rollup Plugin Name: node-resolve

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

This PR fixes the issue #209, where using the resolveOnly option at the node-resolve plugin prevented the resolution of local modules.

[shellscape]

Thanks for the PR!

I might be missing something here, entirely possible, but if this change were successful, wouldn't the deepEqual fail for the only test?

  t.deepEqual(imports, ['@scoped/foo', '@scoped/bar']);

[JaviIG]

Hello @shellscape. I'm the one probably missing something here. For what I understood about the issue #209 is that the resolveOnly option shouldn't prevent resolving local modules.

So in the only.js tests, as the only-local.js fixture is a local module, it should be resolved always, and never imported. If that assumption is right, shouldn't the node-resolve plugin configured either with the test string, or the /^@scoped\/.*$/ regex, never import the only-local.js file?

[shellscape]

Yeah I think I mistook the differences between imports and resolved.

I would like to try and get some test in place that has something we can check that asserts that the local file did resolve. It looks like the lack of warnings is the signal that it's successful but that feels brittle. Nothing to block this PR on but if you can think of anything that'd be cool.

I'd like @lukastaegert to put some quick eyes on this, but it looks good to me so far.

[JaviIG]

Great! I don´t have a deep understanding of the Rollup internal types and properties, so I might be wrong, but after a fast check, I thing we can use the ´PreRenderedChunk.modules´ property, and test if the modules that should be resolved are contained in that dictionary keys. If it works as I think, this might give the tests a little more confidence.

Other option I can think of is searching inside the generated code, but it is far more ugly and fragile than searching inside the imported or resolved modules.

If you have any more ideas I would be more than happy to try them.

[lukastaegert]

Looks good to me. Regarding the question of testing, a stable way to check if a module has been bundled would be to change in the test

const imports = await getImports(bundle);

to

const { output: [{imports, modules}]} = await bundle.generate({ format: 'esm' });

Then modules is an object the keys of which are the resolved included modules, so you could for instance

t.assert(Object.keys(modules).includes(path.resolve('only.js')))

[shellscape]

@JaviIG would you like to try Lukas' test suggestions before we merge?

[JaviIG]

Thank you both! I´ve just uploaded the improved tests. That was the idea I had, but I wasn´t 100% sure about the behavior of the modules property. If there is anything more you think I can improve just tell me :)

[shellscape]