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: Implement IntoOwned for more types #599

Merged
merged 74 commits into from Oct 12, 2023

Conversation

kdy1
Copy link
Contributor

@kdy1 kdy1 commented Oct 10, 2023

I forgot to add derives.

But I'm stuck due to an API design issue.

Is it fine to implement IntoOwned for the selector types? If so, where should the trait definition go?

@kdy1
Copy link
Contributor Author

kdy1 commented Oct 10, 2023

cc @devongovett

@devongovett
Copy link
Member

hmm good question. selectors are annoying. I started by trying to use the existing selectors crate but have needed to make more and more modifications. At some point I'll probably refactor it.

Seems like we have 4 options:

  1. Implement IntoOwned for selector types manually within lightningcss.
  2. Merge selectors into lightningcss and use derive.
  3. Use derive within selectors crate, and modify it so it uses the trait from lightningcss.
  4. Move traits into a separate crate and use derive.

(3) seems like it would be easy but seemed like you had to do some hacking to get derive to import lightningcss from within lightningcss. Is (4) better? Seems like a lot to add a whole new crate just for traits but maybe? Not sure. Any thoughts? I can experiment in a bit as well.

@kdy1
Copy link
Contributor Author

kdy1 commented Oct 11, 2023

I think 4 would be rather easy if you are fine with a new crate. I agree that a new crate can be too much for this... but I think this crate can be generally useful.

@devongovett
Copy link
Member

Actually yeah, moving only IntoOwned to a separate crate is a pretty good idea since it isn't really lightningcss specific. If you want to do that within the repo for now, maybe I can move it to a separate one later on. Not really sure of a good name for it that's available on crates.io... Maybe just into-owned-trait (unfortunately into-owned is taken but unused). If you think of a better one though feel free.

Btw, since it's now a trait, I think a bunch of the special cases (e.g. Option and Box) can also be implemented with generics instead of in the derive macro so it might get a lot simpler.

@kdy1
Copy link
Contributor Author

kdy1 commented Oct 11, 2023

What about static-self?

@kdy1
Copy link
Contributor Author

kdy1 commented Oct 11, 2023

Okay... It seems like it's almost done.

@devongovett
Copy link
Member

What about static-self?

I like it!

@kdy1 kdy1 marked this pull request as ready for review October 11, 2023 19:42
@kdy1
Copy link
Contributor Author

kdy1 commented Oct 11, 2023

I did it 😄

@kdy1
Copy link
Contributor Author

kdy1 commented Oct 11, 2023

Ah. I think the code for derive macro require some cleanup.
Will you do it?

@devongovett
Copy link
Member

Sure, yeah I can take a look. Thanks so much for working on this. It'll be really helpful for some use cases I have in mind too. I'll look into moving these crates into a separate repo at some point and I can add you as a maintainer there too.

@kdy1
Copy link
Contributor Author

kdy1 commented Oct 12, 2023

@devongovett May I ask when a new version will be published?
This is required for some company tasks.

@devongovett devongovett merged commit 1795ac9 into parcel-bundler:master Oct 12, 2023
3 checks passed
@devongovett
Copy link
Member

Done 938fc8c

@kdy1
Copy link
Contributor Author

kdy1 commented Oct 12, 2023

Thank you!

@olishiz
Copy link

olishiz commented Apr 24, 2024

Whoa, that's cool I'm in a part of a Pull Request.

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.

None yet

3 participants