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: add parserConfig option #186

Merged
merged 2 commits into from
Jan 26, 2024
Merged

feat: add parserConfig option #186

merged 2 commits into from
Jan 26, 2024

Conversation

ArnaudBarre
Copy link
Member

Fixes #182

I didn't go with the classic include/exclude because this allow to return parser object and giving full control for the user at very low cost

@ArnaudBarre ArnaudBarre self-assigned this Dec 16, 2023
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bluwy
Copy link
Member

bluwy commented Dec 19, 2023

Personally I'd still lean on include/exclude for consistency and the easier migration from plugin-react. Plus, perhaps one would want the filter function to return more plugin options in the future? Also thinking about Rolldown, there's a chance we could be passing raw glob strings or regex in its API to minimize native<->js calls like esbuild?

@ArnaudBarre
Copy link
Member Author

My opinion was always to wait the maximum to add this kind of option so when implementing in native we don't need complex configuration and can rely on "standards". This is theoretically nice to be able to switch the config on a per file basis, but once you once used esbuild to bundle a complete React codebase without any plugins you feel the cost of being outside of standards and I want to help people be on the right track for sub-second bundle of real world codebases

@XantreDev
Copy link

I also would like to have an opportunity to use swc for deps to unblock usage of swc plugins

@bluwy
Copy link
Member

bluwy commented Dec 20, 2023

@ArnaudBarre I think that's a fair goal when introducing new options. But if we're introducing a new option that's almost identical to how the ecosystem already implements it, I think it's better to reach directly to it. If we waited for the maximum, we'd have to deprecate filter and introduce a migration path which isn't quite nice 🤔

Re-reading the changes, perhaps an option named parserConfig would fit better? filter for me implies returning a boolean. Also if the API is used, you'd also have to manually replicate the existing TS/MDX handling, is that intentional?

@ArnaudBarre
Copy link
Member Author

Yes this is intentional so that it can act as an exclude at the same time and gives you back immediately full control for people that want to oup-out of the default without being complex in the code for us. I'm ok for changing the name so that it's doesn't mislead people that this is can predicate callback

@bluwy
Copy link
Member

bluwy commented Dec 23, 2023

If others are ok I'd prefer renaming the option then 👍 It would also be nice to mention that they need to re-define the config for general TSX/MDX files etc too with this option.

@sapphi-red
Copy link
Member

Renaming the option to prevent misleading sounds good to me.

@ArnaudBarre ArnaudBarre changed the title feat: add filter option feat: add parserConfig option Jan 21, 2024
@ArnaudBarre ArnaudBarre merged commit e936940 into main Jan 26, 2024
@ArnaudBarre ArnaudBarre deleted the filter-option branch January 26, 2024 21:16
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.

Plain Vanilla JS files are excluded from SWC processing/transforms
5 participants