fix(preset-env): Consider browserslist config if env.target not configured #8921

egfx-notifications · 2024-05-06T13:20:51Z

Description:

The env.path option in .swcrc was originally implemented for browserslist, but implementation was (unintentionally?) removed during the migration to browserslist-rs (#2845).
Searching the current directory for browserslist configuration was also dropped in the process.

This PR reimplements searching for browserslist configuration using the lookup functionality provided by browserslist-rs

BREAKING CHANGE:

This may be considered a breaking change since the previous (broken) behavior for unconfigured env.targets was to return a BrowserData struct with all Options set to None. Now the result of a browserslist defaults query is returned instead. This probably was the original behavior with browserslist, though.
Update: Thinking about it again, targets_to_versions() is part of preset_env_bases public API so we should probably consider this a breaking change anyway.

Things to consider:

only works for non wasm targets as browserslist-rs does not provide configuration loading for wasm32 targets (which is probably to be expected that we won't check environment variables or search a filesystem in that case)
feat(preset-env): Let browserslist-rs handle default config path changes the default for an unconfigured env.path from an empty string for wasm targets / current dir for other targets to a None value. browserslist-rs already looks up the current dir if path is None and it felt better to let it handle this case itself
The commit which migrated from browserslist to browserslist-rs mentioned fixing the default_path, but does not mention what was broken and I also did not find any other usage of the path property. If the previous implementation was intentional, just ignore that last commit in the PR, the first two sufficiently address the bug.

Related issue:
Fixes #3365
Implements #3085

changeset-bot · 2024-05-06T13:20:55Z

🦋 Changeset detected

Latest commit: e0be2b6

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

CLAassistant · 2024-05-06T13:20:58Z

All committers have signed the CLA.

CLAassistant · 2024-05-06T13:20:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

kdy1 · 2024-05-06T14:15:27Z

cc @kwonoj

egfx-notifications · 2024-05-06T14:19:00Z

It seems some of the tests fail due to them expecting a specific prerecorded results, but this result has now changed due to the previous default without a target being BrowserData with only None values and now it is BrowserData according to browserslist-rs defaults query, so some stuff that browser now understand natively is not transpiled.

Seems forcing a browserslist query when target is none is not the best approach. I won't try any fixes before hearing your feedback on how to handle this. Should we make the env.path option mandatory if a browserslist config lookup is desired?

honzahruby · 2025-02-26T14:50:16Z

@egfx-notifications Any update on this? 🙏

How about adding some constant to opt in this behavior? Something like env.targets === 'auto' or true.

egfx-notifications · 2025-02-26T15:58:59Z

@egfx-notifications Any update on this? 🙏

No, still waiting on maintainer feedback before I put any more work into this.

How about adding some constant to opt in this behavior? Something like env.targets === 'auto' or true.

This would probably be a elegant non-breaking way.

crates/preset_env_base/src/query.rs

kdy1

Can you rebase?

egfx-notifications · 2025-02-27T10:11:06Z

Can you rebase?

Wait a sec, that rebase is faulty. I'm going to fix this manually asap.

jodaka · 2025-02-27T10:19:33Z

Nice to see some action here! Thanks @egfx-notifications

Defaults to empty PathBuf for wasm32 target, but does not seem to be used anyway

Maybe reconsider this later if this is functionality we want

egfx-notifications · 2025-02-27T16:01:40Z

I think I addressed all of the changes between my original commits and now. Unfortunately I can't run cargo test locally right now as my GLIBC version is too old. Let's see what CI says. I may set up a newer Ubuntu VM tomorrow if necessary.

kdy1 · 2025-02-27T16:04:16Z

No worries, I‘ll update the tests if needed

codspeed-hq · 2025-02-27T23:14:28Z

CodSpeed Performance Report

Merging #8921 will degrade performances by 3.55%

_{Comparing egfx-notifications:fix/browserslist-config-fallback (e0be2b6) with main (edea2c5)}

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 188 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`es/minifier/libs/jquery`	128.9 ms	133.7 ms	-3.55%
⚡	`es/preset-env/usage/builtin_type`	255.4 µs	48.3 µs	×5.3
⚡	`es/preset-env/usage/property`	118.4 µs	25.1 µs	×4.7

.changeset/tidy-pots-report.md

kdy1 · 2025-02-28T04:50:20Z

crates/preset_env_base/src/query.rs

+                    .clone()
+                    .into_os_string()
+                    .into_string()
+                    .unwrap_or_else(|_| panic!("Invalid path \"{:?}\"", path))


I think we should not panic!() here. Can you return an instance of anyhow::Error instead?

kdy1 · 2025-02-28T04:50:43Z

crates/preset_env_base/src/query.rs

+                .with_context(|| "failed to resolve browserslist query from browserslist config")?;
+
+            let versions =
+                BrowserData::parse_versions(distribs).expect("failed to parse browser version");


Same here. We don't have to panic.

kdy1

Thank you, and sorry for a (very) late review!

kdy1

I missed it while reviewing, but while trying to update tests to merge this PR, I found that this PR makes the swc crate ignore jsc.target.
Could you fix it?

You can run UPDATE=1 cargo test --test projects --test tsc --test error_msg from ./crates/swc to update test snapshots.

jsc.target is broken

egfx-notifications requested review from a team as code owners May 6, 2024 13:20

egfx-notifications force-pushed the fix/browserslist-config-fallback branch from 25eb315 to 7a7aaf8 Compare May 6, 2024 13:39

egfx-notifications requested a review from a team as a code owner May 6, 2024 13:39

kdy1 assigned kwonoj May 7, 2024

kdy1 modified the milestone: Planned Feb 26, 2025

kdy1 reviewed Feb 27, 2025

View reviewed changes

crates/preset_env_base/src/query.rs Show resolved Hide resolved

kdy1 assigned kdy1 and unassigned kwonoj Feb 27, 2025

kdy1 added this to the Planned milestone Feb 27, 2025

kdy1 reviewed Feb 27, 2025

View reviewed changes

egfx-notifications force-pushed the fix/browserslist-config-fallback branch from 7a7aaf8 to d731765 Compare February 27, 2025 10:05

egfx-notifications added 4 commits February 27, 2025 11:25

fix(preset-env): Load browserslist config if no swc target configured

0f68ce0

fix(preset-env): Pass through path from swc env config to browserslist

3643ecf

Defaults to empty PathBuf for wasm32 target, but does not seem to be used anyway

feat(preset-env): Let browserslist-rs handle default config path

62f3170

fix(preset-env): Ignore env.path for swc_css_prefixer

a5a35fa

Maybe reconsider this later if this is functionality we want

egfx-notifications force-pushed the fix/browserslist-config-fallback branch from d731765 to a5a35fa Compare February 27, 2025 15:58

Create tidy-pots-report.md

2d35731

kdy1 requested changes Feb 28, 2025

View reviewed changes

fix(preset-env): Prefer anyhow:Error over panic!

e0be2b6

kdy1 previously approved these changes Mar 3, 2025

View reviewed changes

kdy1 reviewed Mar 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(preset-env): Consider browserslist config if env.target not configured #8921

fix(preset-env): Consider browserslist config if env.target not configured #8921

egfx-notifications commented May 6, 2024 •

edited

Loading

changeset-bot bot commented May 6, 2024 •

edited

Loading

CLAassistant commented May 6, 2024 •

edited

Loading

CLAassistant commented May 6, 2024

kdy1 commented May 6, 2024

egfx-notifications commented May 6, 2024

honzahruby commented Feb 26, 2025

egfx-notifications commented Feb 26, 2025

kdy1 left a comment

egfx-notifications commented Feb 27, 2025

jodaka commented Feb 27, 2025

egfx-notifications commented Feb 27, 2025

kdy1 commented Feb 27, 2025

codspeed-hq bot commented Feb 27, 2025 •

edited

Loading

kdy1 Feb 28, 2025

kdy1 Feb 28, 2025

kdy1 left a comment

kdy1 left a comment •

edited

Loading

fix(preset-env): Consider browserslist config if env.target not configured #8921

Are you sure you want to change the base?

fix(preset-env): Consider browserslist config if env.target not configured #8921

Conversation

egfx-notifications commented May 6, 2024 • edited Loading

changeset-bot bot commented May 6, 2024 • edited Loading

🦋 Changeset detected

CLAassistant commented May 6, 2024 • edited Loading

CLAassistant commented May 6, 2024

kdy1 commented May 6, 2024

egfx-notifications commented May 6, 2024

honzahruby commented Feb 26, 2025

egfx-notifications commented Feb 26, 2025

kdy1 left a comment

Choose a reason for hiding this comment

egfx-notifications commented Feb 27, 2025

jodaka commented Feb 27, 2025

egfx-notifications commented Feb 27, 2025

kdy1 commented Feb 27, 2025

codspeed-hq bot commented Feb 27, 2025 • edited Loading

CodSpeed Performance Report

Merging #8921 will degrade performances by 3.55%

Summary

Benchmarks breakdown

kdy1 Feb 28, 2025

Choose a reason for hiding this comment

kdy1 Feb 28, 2025

Choose a reason for hiding this comment

kdy1 left a comment

Choose a reason for hiding this comment

kdy1 left a comment • edited Loading

Choose a reason for hiding this comment

egfx-notifications commented May 6, 2024 •

edited

Loading

changeset-bot bot commented May 6, 2024 •

edited

Loading

CLAassistant commented May 6, 2024 •

edited

Loading

codspeed-hq bot commented Feb 27, 2025 •

edited

Loading

kdy1 left a comment •

edited

Loading