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(preset-env): Consider browserslist config if env.target not configured #8921

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

egfx-notifications
Copy link

@egfx-notifications egfx-notifications commented May 6, 2024

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

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

changeset-bot bot commented May 6, 2024

⚠️ No Changeset found

Latest commit: 7a7aaf8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@CLAassistant
Copy link

CLAassistant commented May 6, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
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.

@egfx-notifications egfx-notifications force-pushed the fix/browserslist-config-fallback branch from 25eb315 to 7a7aaf8 Compare May 6, 2024 13:39
@egfx-notifications egfx-notifications requested a review from a team as a code owner May 6, 2024 13:39
@kdy1
Copy link
Member

kdy1 commented May 6, 2024

cc @kwonoj

@egfx-notifications
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Browserslist not being applied from .browserslistrc or package.json
4 participants