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

refactor: avoid minimist if register receives explicit options #206

Merged
merged 6 commits into from May 2, 2022

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Apr 15, 2022

⚠️ Please merge #205 before this one!

See 3bc36fb for the changes that aren't inherited from #205.

This is a follow-up to #53 (comment). The benefit is avoiding unintentional CLI parsing when tsconfig-paths is embedded into other CLIs, like Vite.

The idea is to avoid parsing --project and -P flags when register is being called programmatically. But when node -r tsconfig-paths/register is used, those flags will still be supported. WDYT?

@jonaskello
Copy link
Member

The register.js file is just a shortcut into the compiled version of register.ts. So I think it would be confusing to put extra code within it. It has the same function as the new exports directive in package.json where you can export as /register.js from within /lib/register.js.

So I think it might be better to keep the register.js->register.ts mirroring and add a new programmatic api to avoid any extra parsing.

@aleclarson
Copy link
Contributor Author

I don't see what's so confusing, but we can do it your way. What should the new function be called?

@jonaskello
Copy link
Member

I looked a bit more into it now. An alternative to a new function might be to have register() disregard command line options when it receives the ExplicitParams as parameter. If you pass ExplicitParams they are the only params used, and the command line is disregarded. So it will only parse the command line if no parameter is passed. Would that work for what you need?

@aleclarson
Copy link
Contributor Author

Sounds good to me! I'll update this PR

@aleclarson aleclarson changed the title refactor: move minimist usage into register.js refactor: avoid minimist if register receives explicit options Apr 20, 2022
@jonaskello
Copy link
Member

@aleclarson I merged #205. This PR now has some conflicts, could you take a look at those and then add an entry to the CHANGELOG? I guess this could be considered breaking since before you could pass explicit params and get the path from the commandline which is no longer possible?

@aleclarson
Copy link
Contributor Author

Would you put this under Added or Fixed or smth else?

@jonaskello
Copy link
Member

jonaskello commented Apr 28, 2022

Do you think this change will be breaking? In that case it should go under "Changed" according to the changelog format. If we're doing a major release there might be some more PRs we could merge.

@aleclarson
Copy link
Contributor Author

LGTM

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

2 participants