-
-
Notifications
You must be signed in to change notification settings - Fork 536
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
Allow specifying "require" option via tsconfig #925
Merged
cspotcode
merged 29 commits into
TypeStrong:master
from
cspotcode:ts-node-config-requires
Jul 30, 2020
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
7e301ef
WIP
cspotcode 73a76f3
WIP
cspotcode 91ca892
WIP tests
cspotcode e11be56
WIP
cspotcode 5b57626
fix
cspotcode d14a028
fix
cspotcode 973a78c
Fix
cspotcode 7dc0009
Addressing PR feedback. Not done yet; just pushing what I have so far
cspotcode 3fa74a5
Pushing fixes to view the gh diff
cspotcode 90e059d
Pushing fixes to view the gh diff
cspotcode 7050acc
Fixes
cspotcode b9b2f67
comments
cspotcode 91f5e13
Cleanup
cspotcode a209bdf
Finalize tsconfig PR
cspotcode 700a90f
Remove ability to specify "requires" in tsconfig
cspotcode 80ea06c
Fix linter failures
cspotcode e7690b9
Revert "Remove ability to specify "requires" in tsconfig"
cspotcode 750dc00
Merge remote-tracking branch 'origin/master' into ts-node-config-requ…
cspotcode 8a5573b
remove duplication from merge
cspotcode 362db15
comments
cspotcode 567582e
fix
cspotcode 3eb3deb
Merge remote-tracking branch 'origin/master' into HEAD
cspotcode bbca6a6
Fix
cspotcode 1c500d5
fix
cspotcode a9ff8c9
fix
cspotcode bd53d10
fix
cspotcode 544d1bf
fix
cspotcode 18566b4
Merge tsconfig and programmatic / CLI `require` arrays; load them in …
cspotcode 6fb4b8f
tests
cspotcode File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
// Extracted from https://github.com/nodejs/node/blob/ec2ffd6b9d255e19818b6949d2f7dc7ac70faee9/lib/internal/modules/cjs/loader.js | ||
// then modified to suit our needs | ||
|
||
const path = require('path'); | ||
const Module = require('module'); | ||
|
||
exports.createRequireFromPath = createRequireFromPath; | ||
|
||
function createRequireFromPath(filename) { | ||
// Allow a directory to be passed as the filename | ||
const trailingSlash = | ||
filename.endsWith('/') || (isWindows && filename.endsWith('\\')); | ||
|
||
const proxyPath = trailingSlash ? | ||
path.join(filename, 'noop.js') : | ||
filename; | ||
|
||
const m = new Module(proxyPath); | ||
m.filename = proxyPath; | ||
|
||
m.paths = Module._nodeModulePaths(m.path); | ||
return makeRequireFunction(m, proxyPath); | ||
} | ||
|
||
// This trick is much smaller than copy-pasting from https://github.com/nodejs/node/blob/ec2ffd6b9d255e19818b6949d2f7dc7ac70faee9/lib/internal/modules/cjs/helpers.js#L32-L101 | ||
function makeRequireFunction(module, filename) { | ||
module._compile('module.exports = require;', filename) | ||
return mod.exports | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 change: 1 addition & 0 deletions
1
tests/tsconfig-options/log-options.js → tests/tsconfig-options/log-options1.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const assert = require('assert') | ||
require('./log-options1') | ||
assert(process.required2) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
process.required1 = true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
require('assert')(process.required1) | ||
process.required2 = true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on doing the require for these options within
index.ts
so configuration flows in a single direction? It might make it easier instead of needing to resolve and pass it back to the CLI which requires. Also, this would ensure we cover the programmatic usage ofregister()
and viats-node/register
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I like that.
The CLI uses
Module._preloadModules
; do you know if that function is doing anything special that will break if we switch to a normalrequire()
call? There's nothing wrong with_preloadModules
but I don't really understand what it does other thanrequire()
stuff.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I recall I tried to change it one time. Feel free to keep the CLI logic as it is and build the internal one separately for now so there's no regression. Here's the implementation in node.js: https://github.com/nodejs/node/blob/ec2ffd6b9d255e19818b6949d2f7dc7ac70faee9/lib/internal/modules/cjs/loader.js#L1400-L1417.
Here's where I had changed it at one point, but I can't seem to find the issue. I should have linked things better in the past 🤕 2e99c50#diff-ee45801c137c3b9f9d8ec7ece340a514R197.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main thing it's trying to ensure is that the
require()
is occurring from the place where you run the script, not wherets-node
lives.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I don't think there's anything wrong with calling
_preloadModules
from withinregister()
, so I'll keep doing that.Should it happen in
register()
or increate()
? I feel likeregister()
makes more sense because that's where we "install" the environment. But source map support is installed increate()
.