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: js/ts dependency inference with file suffix #22041

Merged
merged 6 commits into from
Mar 7, 2025

Conversation

kevin-oliveira-zocdoc
Copy link
Contributor

@kevin-oliveira-zocdoc kevin-oliveira-zocdoc commented Mar 4, 2025

Description

Its possible that we have a file like foo.<suffix>.js, but when importing that file on another .js file, dependency inference wasn't working.

@@ -166,7 +166,7 @@ def _add_extensions(file_imports: frozenset[str], file_extensions: tuple[str, ..
for file_import in file_imports
for string in (
[file_import]
if PurePath(file_import).suffix
if PurePath(file_import).suffix and PurePath(file_import).suffix in file_extensions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we had a file like foo.<suffix>.js, foo.<suffix> was being added to the PathGlobs, but that file doesn't exist, the correct would be to fallback to the else statement and use foo.<suffix>.(<file_extensions>), so I'm adding a check to make sure the suffix is what we would expect coming from the parameters (all js/ts extensions depending on each dependency inference).

rule_runner.write_files(
{
"root/project/src/__generated__/BUILD": "javascript_sources()",
"root/project/src/__generated__/moduleA.generated.js": "",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.generated.js is one use case, but any suffix should be handled correctly.

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! See comment.

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Thanks!

@kevin-oliveira-zocdoc
Copy link
Contributor Author

macos test suite failed with:

native_engine.IntrinsicError: Server error (500) downloading file pex from https://github.com/pex-tool/pex/releases/download/v2.33.1/pex

can we re-run?

@tdyas tdyas added the category:bugfix Bug fixes for released features label Mar 7, 2025
@tdyas
Copy link
Contributor

tdyas commented Mar 7, 2025

May I assume this should be backported to the 2.25.x branch?

@tdyas tdyas added this to the 2.25.x milestone Mar 7, 2025
@benjyw benjyw merged commit b2b15a4 into pantsbuild:main Mar 7, 2025
24 checks passed
@WorkerPants
Copy link
Member

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.25.x

Successfully opened #22056.


Thanks again for your contributions!

🤖 Beep Boop here's my run link

tdyas pushed a commit that referenced this pull request Mar 7, 2025
…) (#22056)

# Description

Its possible that we have a file like `foo.<suffix>.js`, but when
importing that file on another .js file, dependency inference wasn't
working.

Co-authored-by: Kevin Oliveira <[email protected]>
@kevin-oliveira-zocdoc
Copy link
Contributor Author

thanks @benjyw @tdyas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants