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

Expose requires #10

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Expose requires #10

wants to merge 5 commits into from

Conversation

developit
Copy link

@developit developit commented Oct 4, 2020

This adds a third requires[] property to the output object in both modes. It's an Array of {s:number,e:number} start/end offsets, like those of es-module-lexer. All require() calls are added to the Array, including re-exports, and are not de-duplicated since the value is their source position.

const source = `foo=require("foo")`;
const { requires } = parse(source);
requires; // [{ s: 13, e: 16 }]
source.substring(requires[0].s, requires[0].e);  // foo

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

With both yourself and @kriskowal now suggesting this I understand it could be useful, so I want to just reexplain my resistance here.

My primary concern is being able to draw a line on where this future would go in future and what the promise of the feature is. It becomes easy from here to take a PR for module.require, and then just require.resolve and then just createRequire tracking and then just dynamic require expressions, and then just wrapper opt outs in cases where require is part of the internal bundle, and then just __non_webpack_require__ and then just tracking __dirname and then just tracking __filename because each step alone is just a small addition to do. It is pushing the project down this path that I want to explicitly resist because this project is a Node.js project for exports tracing and all the performance and code and maintenance focus should be on that.

I think I would rather suggest forking this project with your own additions unless I can be convinced that this isn't a slippery slope with the above additions as the cases get hit. But I really strongly feel it is.

lexer.js Outdated Show resolved Hide resolved
lexer.js Outdated Show resolved Hide resolved
src/lexer.c Outdated Show resolved Hide resolved
src/lexer.c Outdated Show resolved Hide resolved
test/_unit.js Outdated Show resolved Hide resolved
test/_unit.js Outdated Show resolved Hide resolved
test/_unit.js Outdated Show resolved Hide resolved
Co-authored-by: Guy Bedford <[email protected]>
@kriskowal
Copy link

@guybedford I’ve made a fork of this and now begin the process of catching the fork up with changes that have occurred here since. This is of course an absurdly expensive exercise in comparison to pooling our effort here. To the upside of pooling our effort, I did find and fix bugs in the process of linting and providing the minimal TypeScript annotations for our purposes. I don’t doubt that the slope might slip a little, but I would expect it to be a shallow slope with a near bottom. For one, we will certainly never need to analyze __dirname and __filename. endojs/endo#666

@guybedford
Copy link
Collaborator

@kriskowal nice to hear. es-module-lexer is a fork to also keep in sync and so far it hasn't been too bad at all to maintain that. If you hit any cases worth upstreaming please do consider it! Would be great to collaborate on keeping this project up to date. Also, if you feel like tipping the old GitHub sponsors account that would always be very much appreciated :)

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.

3 participants