Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1004: resolve config relative to realpath of entrypoint #1009

Merged
merged 4 commits into from
Apr 20, 2020

Conversation

cspotcode
Copy link
Collaborator

Fixes #1004.

@cspotcode cspotcode force-pushed the ab/resolve-entrypoint-symlink branch 2 times, most recently from 768c401 to f7a9d62 Compare April 12, 2020 23:08
@cspotcode cspotcode force-pushed the ab/resolve-entrypoint-symlink branch from f7a9d62 to 5f28488 Compare April 12, 2020 23:10
@cspotcode cspotcode requested a review from blakeembrey April 12, 2020 23:13
@@ -251,7 +252,21 @@ function getCwd (dir?: string, scriptMode?: boolean, scriptPath?: string) {
throw new TypeError('Script mode cannot be combined with `--dir`')
}

return dirname(scriptPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I’d be more comfortable noting that we only support symlinked directories instead of doing this, but up to you. If we only support directories it would become easy. Alternatively we can implement the resolver in a loop ourselves instead of messing with require.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might not even have TSX, JSX support extensions added either, so it’s another difference here. You could expose the function that creates a list of extensions, then just loop over that list and resolve like node.js does since we don’t need the extra require logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to support loading a directory containing an index.* file, or a directory containing a package.json with a main field pointing to another file? E.g. require('./directory-containing-package-json')?

The chicken-and-egg problem with file extensions is a real bummer. I'll try to come up with some realistic examples of this causing problems in real life.

The only reason I was hoping to support symlinked scripts is so they can be put on the PATH. npm and yarn do this a lot, and I can see it coming up in monorepo situations. (lerna, yarn2)

@cspotcode
Copy link
Collaborator Author

cspotcode commented Apr 13, 2020

Here is my analysis for if --script-mode ambiguous config resolution will be a problem in real-world scenarios.

Things we don't know that make it ambiguous

  • whether or not .tsx or .jsx are enabled
  • whether .ts(x) takes precedence over .js(x)

When resolution might match 2x different files, we may follow the wrong one, which may lead us to discover the wrong config.

Ways ambiguity can happens

  • 2x matched files are peers of each other, e.g. ./somedir/foo.jsx and ./somedir/foo.ts, and symlink to different target directories
  • file is peer with directory of the same name, e.g. /somedir/foo.tsx and /somedir/foo/package.json

Contrived examples

  • foo.jsx and foo.ts peers in same directory, symlinked to different target directories. preferTsExts and allowJs determine which one is resolved.
    • This seems extremely rare. Sure, peers might exist, but they either wouldn't be symlinks, or they'd point to the same target directory
  • foo.jsx and foo/ in same location. Do we look at foo.jsx or foo/index.*, or whatever foo/package.json says? (foo.jsx takes precedence, but jsx might not be enabled)

Real-world examples

None

I can't think of a real-world situation where the contrived examples will occur.

Mitigation strategies

Based on the above, I don't think ambiguity is a real problem. But if we want to avoid issues, here are a few strategies:

  • Do not support symlinked files, only symlinked directories

    • Breaks the npm link use-case
    • May break monorepos (yarn 2 and lerna) where one local package exports binary executed by others via package scripts?
  • Detect and throw an error whenever ambiguity affected config resolution

    • Throw when directory we resolve initially for config discovery is different than directory resolved to run the script (in all other cases, ambiguity has no real effect)
    • When anyone hits ambiguity in the real world, we will get a ping on our issue tracker
    • Only do this test when TS_NODE_DEBUG is enabled, to avoid impacting performance?
  • Do nothing; allow ambiguity

    • Affected users can specify full path to avoid ambiguity

@cspotcode cspotcode merged commit a549b5a into master Apr 20, 2020
@cspotcode cspotcode deleted the ab/resolve-entrypoint-symlink branch May 9, 2020 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

realpath of entry-point script is not resolved
2 participants