Path mapping part 2

[cspotcode]

@charles-allen I'm creating this pull request so we can discuss and review the tests you are adding for path-mapping.

Closes #704
Closes #1585
Closes #1515

This is a continuation of the work started in #1585

Below is the list of tests, copied from #1585 (review)

• test when baseUrl is set and paths is omitted
  • should still map import "foo" to ${baseUrl}/foo
• test when baseUrl is set and paths maps "*": ["lib/*"]
  • import "foo" should not attempt ${baseUrl}/foo; should only attempt ${baseUrl}/lib/foo
• test when specifier matches a paths pattern but must fallback to node_modules
  • for example, paths: {"*": ["src/*"]}, import "lodash" should fallback to node_modules/lodash
• test when specifier matches a paths pattern but must fallback to node built-in module
  • same as above but for import "fs"
• When mapping candidate resolves to a .d.ts, resolution should be skipped; try next one
• test that external modules (node_modules/*) are unaffected by mappings
  • if path mappings map "lodash" to "./src/lodash" and node_modules/external-library/index.js does import "lodash", it should not get ./src/lodash
• test that mapping does not apply to relative imports
  • import "./foo" should not be mapped even if "*" mapping exists
  • import "/absolute/path/foo" should not be mapped even if "*" mapping exists

[charles-allen]

@cspotcode - I think I've finished the tests 🥳 🎉

A lot of the new tests are currently failing. I think:

• a lot are imports that don't have an explicit file extension (ex: import foo from 'foo') that fail, even when from foo.js would succeed -- I thought that with node resolution, dropping the extension is ok?
• a few are expected errors failing because the error message format is different to expected

I'm not 100% sure what import xyz from '/xyz' is supposed to do -- does it mean the root directory of the entire computer? Do you want to test that somehow? Maybe using resolve & __dirname?

Do you have any more ideas for tests? Or anything else that needs cleaning up?

[cspotcode]

I thought that with node resolution, dropping the extension is ok?

Node only allows dropping some extensions when using require() or when using --experimental-specifier-resolution=node. For example, require() is allowed to omit a .js extension but not .cjs or .mjs extensions. Our ESM loader maps from .js imports to .ts where appropriate, but we do not have a corresponding require() resolver hook to do the same mapping. This means ESM can import from './foo.js' to load foo.ts, but require('./foo.js') will fail to map to ./foo.ts. I am working on adding this mapping behavior to require() in another pull request.

a few are expected errors failing because the error message format is different to expected

Does this mean we need to update the tests to understand different variants of error messages?

I'm not 100% sure what import xyz from '/xyz'

Yeah, that's probably testing absolute paths, to make sure they are not accidentally treated as being relative or dependencies. Where do you see /xyz? In the code or in a comment?

[cspotcode]

Also, congrats on finishing the tests! I will review them this week.

[charles-allen]

Also, congrats on finishing the tests! I will review them this week.

I spoke too soon. I think on review, I actually haven't finished these:

• test when specifier matches a paths pattern but must fallback to node_modules
  • for example, paths: {"*": ["src/*"]}, import "lodash" should fallback to node_modules/lodash

I need to add paths for some-cjs-dependency & some-esm-dependency that miss, so that it's actually falling-back

• test when specifier matches a paths pattern but must fallback to node built-in module
  • same as above but for import "fs"

I need to add a path for fs that misses local files

• test that external modules (node_modules/*) are unaffected by mappings
  • if path mappings map "lodash" to "./src/lodash" and node_modules/external-library/index.js does import "lodash", it should not get ./src/lodash

I need to add a path for lodash that doesn't miss; I currently have it hitting based on the baseUrl only

• test that mapping does not apply to relative imports
  • import "/absolute/path/foo" should not be mapped even if "*" mapping exists

Still need to write this one!

[charles-allen]

I thought that with node resolution, dropping the extension is ok?

Node only allows dropping some extensions when using require() or when using --experimental-specifier-resolution=node. For example, require() is allowed to omit a .js extension but not .cjs or .mjs extensions. Our ESM loader maps from .js imports to .ts where appropriate, but we do not have a corresponding require() resolver hook to do the same mapping. This means ESM can import from './foo.js' to load foo.ts, but require('./foo.js') will fail to map to ./foo.ts. I am working on adding this mapping behavior to require() in another pull request.

Should I be explicitly adding .js everywhere then?

a few are expected errors failing because the error message format is different to expected

Does this mean we need to update the tests to understand different variants of error messages?

I copied the error messages from the example tests. I wasn't sure if you intended to update the tests to match the implementation or the implementation to match the tests. If it's the former, I could do that!

I'm not 100% sure what import xyz from '/xyz'

Yeah, that's probably testing absolute paths, to make sure they are not accidentally treated as being relative or dependencies. Where do you see /xyz? In the code or in a comment?

It's the last requested test case! (I haven't done it yet)

[charles-allen]

That sounds like a lot left, but I think the above are small fixes, so I think I'm pretty close -- I'll try to do another push tomorrow night to wrap it up :)

[cspotcode]

Should I be explicitly adding .js everywhere then?

Hmm, I'm not sure, I'll try to answer that when I review later this week.

I copied the error messages from the example tests. I wasn't sure if you intended to update the tests to match the implementation or the implementation to match the tests. If it's the former, I could do that!

Also not sure about this one, I'll check when I review.

It's the last requested test case! (I haven't done it yet)

I see. Yeah, for that one, we need to test that path mapping does not apply to absolute and relative paths.

• import "./foo" should not be mapped even if "*" mapping exists.
  • For example, even if foo maps to projectroot/mapped/foo.js, ./foo should not be mapped because it is a relative path
• import "/absolute/path/foo" should not be mapped even if "*" mapping exists
  • For example, even if foo maps to projectroot/mapped/foo.js, /foo should not be mapped because it is an absolute path

[cspotcode]

Coming back to this:

Should I be explicitly adding .js everywhere then?

Is it feasible to add .js when importing, and omit it for require()s? Or is that what you've already done?

It would allow us to run the tests before we implement .js->.ts mapping in require(). ...if that makes sense.

I copied the error messages from the example tests.

What examples are you referring to? Did they come from me? It's possible that I forgot providing examples somewhere.

If the error messages already being raised look appropriate, then we should update the tests to match the errors being raised.

If you think the error messages being raised are incorrect, inadequate, or should be adjusted, then let's talk about that.

[cspotcode]

I use the postfix as syntax for all type assertions since it's compatible with TSX files. I realize this project doesn't have TSX, but that's just the convention I use everywhere. So we should stick to that for consistency.

[charles-allen]

What examples are you referring to? Did they come from me?

They were in the original PR before I started

If the error messages already being raised look appropriate, then we should update the tests to match the errors being raised.
If you think the error messages being raised are incorrect, inadequate, or should be adjusted, then let's talk about that.

Ok, I will review the current errors & get back to you :)

[cspotcode]

Closing now that we can all collaborate on #1585
But should be sure that we address all tasks from this thread.