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

fix: dont remove imports in nodejs env #921

Merged
merged 1 commit into from
May 6, 2024

Conversation

KuznetsovRoman
Copy link
Member

@KuznetsovRoman KuznetsovRoman commented May 3, 2024

What is done

Fixed removing extra imports in nodejs env.

Copy link
Member

@shadowusr shadowusr left a comment

Choose a reason for hiding this comment

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

Looks good to me except for the opts naming in test parser.

src/test-reader/test-parser.js Show resolved Hide resolved
],
};

const customIgnoreImportsPlugin = (): PluginObj => ({
Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud, I wonder if this was being applied to nodejs code by genuine oversight or for a reason... But since before having CT, everything worked fine without this code, it should work fine now, too

Copy link
Member Author

@KuznetsovRoman KuznetsovRoman May 6, 2024

Choose a reason for hiding this comment

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

I think he just didn't thought ".tsx" files could be read in "nodejs" env, without explicit file endings.
As we in "nodejs" env don't know statically "which extension" the file have, i think it is better to just don't do anything, as it produces hard to debug problems (the problem only reproduces when we have ".tsx" file which has "import" of file with "." in its name).

I suggest to think about it later, then with CT contributor we could discuss this. For example, we could only remove unnamed imports (like import "styles.css") and don't touch named imports (like import myModule from "foo.css"), or have specific blacklist of extensiong (for example, remove import foo from "styles.css", but don't remove import {foo} from "list.constants")

@KuznetsovRoman KuznetsovRoman force-pushed the TESTPLANE-13.babel_imports branch 3 times, most recently from f5680d2 to a157c29 Compare May 6, 2024 14:25
@KuznetsovRoman KuznetsovRoman force-pushed the TESTPLANE-13.babel_imports branch from a157c29 to f132eeb Compare May 6, 2024 14:28
@KuznetsovRoman KuznetsovRoman merged commit d2e06e2 into master May 6, 2024
2 checks passed
@KuznetsovRoman KuznetsovRoman deleted the TESTPLANE-13.babel_imports branch May 6, 2024 14:44
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.

2 participants