Skip to content
This repository was archived by the owner on Dec 15, 2023. It is now read-only.

Fix expression-file by using require and path.resolve - #168

Open
Amin Ya (aminya) wants to merge 2 commits into
microsoft:masterfrom
aminya:fix-expression-file
Open

Fix expression-file by using require and path.resolve#168
Amin Ya (aminya) wants to merge 2 commits into
microsoft:masterfrom
aminya:fix-expression-file

Conversation

@aminya

@aminya Amin Ya (aminya) commented Apr 22, 2021

Copy link
Copy Markdown

Use it until this is merged:

npm install -g @aminya/dts-gen

dts-gen --expression-file=./your_file.js

Description

It fixes expression-file to support any JavaScript file as the input.

Fixes #58
Fixes #65
Fixes #105

Reasoning

It seems that eval acts buggy when it comes to resolving dependencies. But require doesn't have this issue as it is just what Node uses by default. Also, path.resolve is needed before passing the file path to require.

Suggestion

If I could, I would have renamed expression-file to file and renamed the current file argument to output-file. The naming of the arguments is inaccurate and confusing.

@aminya Amin Ya (aminya) changed the title Fix expression-file by using require and path.resolve Fix expression-file by using require Apr 22, 2021
@aminya Amin Ya (aminya) changed the title Fix expression-file by using require Fix expression-file by using require and path.resolve Apr 22, 2021
@ghost

Deleted user (ghost) commented Apr 22, 2021

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@aminya

Copy link
Copy Markdown
Author

Orta Therox (@orta) Nathan Shively-Sanders (@sandersn) Could you take a look at this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To fix #58, I would prefer to add a new option instead of changing what --expression-file means. It does basically what it says -- generates types based on evaluating the last expression in a file. Perhaps it should have the name --last-expression-in-file by default.

It's likely that the fix for #65 will be more complicated, since it will have to retain the eval-last-expression behaviour of --expression-file.

I don't see how this fixes #105. There aren't any requires in that file at all, it's just an IIFE. Can you explain?

@aminya

Amin Ya (aminya) commented Apr 25, 2021

Copy link
Copy Markdown
Author

To fix #58, I would prefer to add a new option instead of changing what --expression-file means.

I wanted to call this --file but it is already taken (it should have been --output-file. Maybe --input-file is a good name.

It's likely that the fix for #65 will be more complicated, since it will have to retain the eval-last-expression behaviour of --expression-file.

There is no such requirement. People use --expression-file as a workaround because there is no other way to generate types for a file. Even if we removed this option, I doubt anyone would complain.

@aminya

Copy link
Copy Markdown
Author

Nathan Shively-Sanders (@sandersn) Should we fix #169?

@Aericio

Aericio commented Jul 26, 2022

Copy link
Copy Markdown

How can I build and try this branch?

@aminya

Copy link
Copy Markdown
Author

I can publish this on npm if you like. It is more maintained than dts-gen.

@Aericio

Aericio commented Jul 28, 2022

Copy link
Copy Markdown

That'd be great.

@aminya

Amin Ya (aminya) commented Aug 23, 2022

Copy link
Copy Markdown
Author

Here you go. I have published it until this is merged:

npm install -g @aminya/dts-gen

dts-gen --expression-file=./your_file.js

@tomaszs

Copy link
Copy Markdown

Anyone?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

4 participants