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

accept require.resolve implementation as option to importLocation et al. #1144

Merged
merged 7 commits into from
Jun 28, 2022

Conversation

naugtur
Copy link
Member

@naugtur naugtur commented Apr 6, 2022

Receiving the implementation externally makes more sense than supporting require.resolve functionality endoEndo.

This PR accepts the implementation and, if not provided, defaults to a similar error Node would throw.

@naugtur naugtur marked this pull request as draft April 6, 2022 10:41
@naugtur naugtur force-pushed the naugtur-require-resolve branch 2 times, most recently from 793d7b0 to c323f1b Compare June 22, 2022 11:34
@@ -87,6 +88,8 @@ export function scaffold(

const application = await loadLocation(readPowers, fixture, {
dev: true,
requireResolve: (from, specifier, options) =>
createRequire(from).resolve(specifier, options),
Copy link
Member Author

Choose a reason for hiding this comment

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

I like how easy it is to provide the implementation.
A more secure implementation would use options.paths to limit the search space and guard return values by checking with String.startsWiththat it's not reaching beyond the project folder.

A cache lookup of known needed paths for specifiers is also an option.

@naugtur naugtur changed the title draft of require.resolve implementation accept require.resolve implementation as option to importLocation et al. Jun 22, 2022
@naugtur naugtur marked this pull request as ready for review June 22, 2022 12:09
@naugtur naugtur force-pushed the naugtur-require-resolve branch 2 times, most recently from 99d349c to 71e7adb Compare June 22, 2022 12:38
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

I would like to fold requireResolve into the optional properties of readPowers. Otherwise, this looks good to me.

};
if (readPowers && readPowers.requireResolve) {
require.resolve = freeze((specifier, options) =>
readPowers.requireResolve(location, specifier, options),
Copy link
Member Author

Choose a reason for hiding this comment

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

Now all that's left to do is convince Typescript I can call this function if I check the field exists even thought it's optional on readPowers

Copy link
Member Author

Choose a reason for hiding this comment

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

it stopped complaining when I did this

 if (typeof readPowers === 'object' && readPowers.requireResolve) {
    const { requireResolve } = readPowers;
    require.resolve = freeze((specifier, options) =>
      requireResolve(location, specifier, options),
    );

I don't appreciate what TS is trying to do for me here

@mhofman
Copy link
Contributor

mhofman commented Jun 23, 2022

I'm not sure I have much context to review this PR. Is there anything specific you'd like me to take a look at?

@naugtur
Copy link
Member Author

naugtur commented Jun 23, 2022

I'm not sure I have much context to review this PR. Is there anything specific you'd like me to take a look at?

@mhofman Automatons said you had overlap with the codebase that this touches. All I'm looking for is a skim and a veto if I did something you consider harmful.

@naugtur
Copy link
Member Author

naugtur commented Jun 24, 2022

@kriskowal This is ready IMHO, I need the final review.

@mhofman
Copy link
Contributor

mhofman commented Jun 24, 2022

@mhofman Automatons said you had overlap with the codebase that this touches

Automatons are mistaken, I'll remove myself as reviewer.

@mhofman mhofman removed their request for review June 24, 2022 19:23
Comment on lines +179 to +181
{
readPowers,
},
Copy link
Member

Choose a reason for hiding this comment

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

Is this envelope still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

While not necessary, it's likely to save some people a bunch of time I lost when adding more items to parse arguments and wondering why they're not getting passed to the parseCjs function because it's wrapped along the way.
I'd like to keep it.

@naugtur naugtur force-pushed the naugtur-require-resolve branch 2 times, most recently from 50abfba to a3e5d2e Compare June 28, 2022 12:20
@naugtur naugtur enabled auto-merge June 28, 2022 12:36
@naugtur naugtur merged commit 8077e21 into master Jun 28, 2022
@naugtur naugtur deleted the naugtur-require-resolve branch June 28, 2022 12:43
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.

None yet

3 participants