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

feat(biome_js_parser): support ts resolution-mode import #3462

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

fireairforce
Copy link
Contributor

@fireairforce fireairforce commented Jul 17, 2024

Summary

closes: #2115

Test Plan

@github-actions github-actions bot added A-Parser Area: parser L-JavaScript Language: JavaScript and super languages labels Jul 17, 2024
@fireairforce fireairforce changed the title fix(js-parser): support ts resolution-mode import (WIP)fix(js-parser): support ts resolution-mode import Jul 17, 2024
Copy link

codspeed-hq bot commented Jul 17, 2024

CodSpeed Performance Report

Merging #3462 will degrade performances by 6.18%

Comparing fireairforce:fix-2115 (f56ef4f) with main (ea7d35c)

Summary

❌ 1 regressions
✅ 100 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main fireairforce:fix-2115 Change
react.production.min_3378072959512366797.js[cached] 1.9 ms 2 ms -6.18%

@Conaclos
Copy link
Member

@fireairforce Are you still interested in the PR?

@fireairforce
Copy link
Contributor Author

@Conaclos i will update this later~

@fireairforce fireairforce force-pushed the fix-2115 branch 2 times, most recently from 15d0bc2 to a741521 Compare October 8, 2024 12:23
@fireairforce fireairforce changed the title (WIP)fix(js-parser): support ts resolution-mode import feat(biome_js_parser): support ts resolution-mode import Oct 8, 2024
@fireairforce fireairforce force-pushed the fix-2115 branch 2 times, most recently from d95fc97 to 0d01dd6 Compare October 11, 2024 02:47
@github-actions github-actions bot added A-Formatter Area: formatter A-Tooling Area: internal tools labels Oct 11, 2024
@fireairforce fireairforce force-pushed the fix-2115 branch 2 times, most recently from e35e118 to 4939375 Compare October 14, 2024 05:44
@@ -1,3 +1,4 @@
import()
import(...["foo"])
import("foo", { assert: { type: 'json' } }, "bar")
import("foo", { with: { type: 'json' } }, "bar")
Copy link
Member

Choose a reason for hiding this comment

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

This is now considered correct? We should move it to ok/import_call.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.

No, that is wrong, it seems that import_attributes only have two params: https://github.com/tc39/proposal-import-attributes?tab=readme-ov-file#dynamic-import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you can provide some reference documentation about this place?

@fireairforce
Copy link
Contributor Author

this mr is ready for review~

Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

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

Thanks!

@Conaclos Conaclos merged commit 30a016b into biomejs:main Oct 17, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Formatter Area: formatter A-Parser Area: parser A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Does not support resolution-mode for typeof import
2 participants