Path mapping part 2 #1664

cspotcode · 2022-03-01T02:13:20Z

@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

The ESM loader now resolves import paths using TypeScripts [path mapping][1] feature. [1]: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>

Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>

charles-allen · 2022-03-28T20:09:19Z

@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 · 2022-03-28T20:17:34Z

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 · 2022-03-28T20:17:57Z

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

charles-allen · 2022-03-28T20:21:13Z

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 · 2022-03-28T20:23:43Z

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 · 2022-03-28T20:27:13Z

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 · 2022-03-28T20:37:24Z

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 · 2022-04-16T18:10:57Z

src/test/path-mapping.spec.ts

+    baseDir: 'esm-path-mapping',
+    command: CMD_ESM_LOADER_WITHOUT_PROJECT,
+  },
+};


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 · 2022-04-17T05:44:51Z

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 · 2022-05-18T21:13:01Z

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

geigerzaehler and others added 30 commits December 29, 2021 15:28

Add path mapping support to ESM loader

3be1fde

The ESM loader now resolves import paths using TypeScripts [path mapping][1] feature. [1]: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>

fixup! Add path mapping support to ESM loader

c542d98

fixup! fixup! Add path mapping support to ESM loader

0cbfa6c

fixup! fixup! fixup! Add path mapping support to ESM loader

e7082b1

Merge remote-tracking branch 'origin/main' into path-mapping

69a397b

address code review comments

2fcb982

Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>

map esm paths in all included files

48f5262

Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>

improve error message when mapped module is not found

a19454b

Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>

Review changes; add CommonJS path mapping

32e26e8

fix failing tests

362935b

add path mapping to docs

e573fd7

add flag to enable/disable path mapping in the two loaders

0012b22

fix windows tests?

b6352e3

fix windows tests?

85643fd

Merge remote-tracking branch 'origin/main' into path-mapping

9c46688

add path mapping option to docs

bede1b6

changes

80746c2

replace equal (deprecated) with strictEqual

885b7b1

extract shared tsconfig

c3dbe73

move import targets 2-deep (to support combined baseUrl + * path)

8c5fd23

test: baseUrl + no paths

e66d236

test: baseUrl + * path

23f40b1

test: fallback to node_modules

54fdbba

clean up destructuring

78652b2

fix setting project

b8e6fb7

test: fallback to built-in

2973399

avoid space in command & add comment

5ffd905

fix setting project and PATH

d200301

test: skip type-defs

c621af0

test: external imports ignore paths

ec038c1

charles-allen added 15 commits March 29, 2022 00:18

move base 2 deep, so star path can be used in addition to base

8e9b117

tests: non-relative imports

806ce74

fix import extensions

0fd8a5c

tests: imports from js, jsx, tsx

02818e3

rename under-base to below-base (nicer lexical sort)

6faedd9

tests: relative imports

64bb679

tests: import invalid path

058d2e2

test: should not use star-path to resolve relative import

4a3b6b4

tests: basic path mapping

dc7e950

fix import style

c5ea9b0

tests: map using first available candidate

7bfe31a

tests: more specific path; static path

c8c35ca

comment out file-system-base-relative import

99ca516

tests: mapping from js, jsx, tsx files

4fefc57

clean up (delete all old tests)

3a0067e

cspotcode commented Apr 16, 2022

View reviewed changes

cspotcode mentioned this pull request Apr 29, 2022

esm: add support for loaders cucumber/cucumber-js#1844

Closed

cspotcode mentioned this pull request May 9, 2022

"Recipes" and "Quick Start:" add a general project config example #1742

Open

cspotcode added 2 commits May 18, 2022 15:51

tweaks before pulling in the latest main branch

78e3eda

Merge remote-tracking branch 'origin/main' into path-mapping

4f5bc35

cspotcode closed this May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path mapping part 2 #1664

Path mapping part 2 #1664

cspotcode commented Mar 1, 2022 •

edited

charles-allen commented Mar 28, 2022

cspotcode commented Mar 28, 2022

cspotcode commented Mar 28, 2022

charles-allen commented Mar 28, 2022

charles-allen commented Mar 28, 2022

charles-allen commented Mar 28, 2022

cspotcode commented Mar 28, 2022

cspotcode left a comment

cspotcode Apr 16, 2022

charles-allen commented Apr 17, 2022

cspotcode commented May 18, 2022

Path mapping part 2 #1664

Path mapping part 2 #1664

Conversation

cspotcode commented Mar 1, 2022 • edited

charles-allen commented Mar 28, 2022

cspotcode commented Mar 28, 2022

cspotcode commented Mar 28, 2022

charles-allen commented Mar 28, 2022

charles-allen commented Mar 28, 2022

charles-allen commented Mar 28, 2022

cspotcode commented Mar 28, 2022

cspotcode left a comment

Choose a reason for hiding this comment

cspotcode Apr 16, 2022

Choose a reason for hiding this comment

charles-allen commented Apr 17, 2022

cspotcode commented May 18, 2022

cspotcode commented Mar 1, 2022 •

edited