Skip to content

fix: ignore .ts and .js extensions form imports #437

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

Merged
merged 3 commits into from
May 31, 2024

Conversation

nohehf
Copy link
Contributor

@nohehf nohehf commented May 27, 2024

Simple quick fix for #435, this can be evolved further but I believe will work in most currently broken cases.

@nohehf nohehf requested a review from a team as a code owner May 27, 2024 08:22
Copy link
Collaborator

@hendrikvanantwerpen hendrikvanantwerpen left a comment

Choose a reason for hiding this comment

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

I think this is the right approach, but the regex seems off. See my comments below.

"([^/]+)/?" {
; ignore .js and .ts extensions on imports, as ts allows this
; this might lead to false positives
"([^/|(\.j|ts)]+)/?" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the idea is correct, bit this doesn't look quite right. What about this?

Suggested change
"([^/|(\.j|ts)]+)/?" {
"([^/]+)(/|\.ts|\.tsx|\.js|\.jsx\)?" {

Copy link
Contributor Author

@nohehf nohehf May 27, 2024

Choose a reason for hiding this comment

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

Actually my regex is a bit of a hack but as .js (and .ts, ...) are included in this broad pattern: ([^/]+) it will be included in the first group if this is optional: (/|\.ts|\.tsx|\.js|\.jsx\) (eg. it will still match the extention). Working on something cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding lazy matching with ? in the first group seems to fix the issue:
^([^/]+?)(\.js|\.ts|\.jsx|\.tsx)?$seems ok, updating the pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the regex parser does not support non greedy matching ? (...?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see the problem! An alternative might be to keep the original regex, and strip any of the extensions off inside the body using (replace ...). That might be easier.

Copy link
Contributor Author

@nohehf nohehf May 30, 2024

Choose a reason for hiding this comment

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

I also tried a way using regexes and replace, but it seems the regex in tsg does not strictly behave as in rust:
Given the regex \.[^\.\\/]+$

scan "./bar" {
    "\.[^\.\\/]+$" {
      print $0
    }
 }

Outputs /bar
But using the same regex in rust it does not match (as expected):
https://regex101.com/r/Rwm8lq/1 (check the unit tests tab)
(also tested on https://rustexp.lpil.uk/)

Copy link
Contributor Author

@nohehf nohehf May 30, 2024

Choose a reason for hiding this comment

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

I believe the implementation of path-dir is kind of wrong:

functions.add(
    "path-dir".into(),
    path_fn(|p| p.parent().map(|s| s.as_os_str().to_os_string())),
);

For instance I expect the dir of . to be . and the dir of .. to be .., but their parents are both None.

Copy link
Contributor Author

@nohehf nohehf May 30, 2024

Choose a reason for hiding this comment

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

I got it to work by removing the explicit extensions (.js, .ts, .jsx, .tsx, not all extensions) with: (path-normalize (replace mod_path "\.(js|ts|jsx|tsx)$" "")), but the questions above still remain and should be considered I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(tho this can be merged I believe)

Copy link
Collaborator

Choose a reason for hiding this comment

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

My kingdom for a small coherent theory of paths 😭. Thanks for figuring out these cases. I'll put them in an issue so we document this in a more visible place.

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