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

Make work with yarn berry #28

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

Make work with yarn berry #28

wants to merge 2 commits into from

Conversation

francisu
Copy link

@francisu francisu commented Jun 1, 2024

This resolves issue #26. It has been tested with sveltekit.

@wooorm
Copy link
Owner

wooorm commented Jun 1, 2024

I’m sorry but as mentioned in 23 and 26 this is not enough, I can’t accept this.
If require.resolve is enough for your needs, then I recommend that you use that.

@francisu
Copy link
Author

francisu commented Jun 1, 2024

I’m sorry but as mentioned in 23 and 26 this is not enough, I can’t accept this. If require.resolve is enough for your needs, then I recommend that you use that.

Can you explain why "this it not enough"? I have read through the comments on issue #23 and don't see how they preclude this fix.

This fix resolves issue #26 as it has been tested with Svelte kit and it's using the document PnP API. It also is benign for non-PnP users since it's protected by the if statement.

The tests for import-meta-resolve pass with this fix, so I don't see the downside, since import-meta-resolve is broken when using yarn berry presently.

@wooorm
Copy link
Owner

wooorm commented Jun 2, 2024

Can you explain why "this it not enough"

import.meta.resolve and this package do particular things.
require.resolve does different things.
If require.resolve was what import.meta.resolve/this package did, they would not exist.

it has been tested with Svelte kit

And what did you test?
I presume that you tested very little, because import.meta.resolve and require.resolve are very different, so they should not be interchangeable.
If this patch works for those cases, you don’t need import-meta-resolve there, but could use require.resolve there directly. Why don‘t you PR that to svelte kit?

it's using the document PnP API

Can you please clarify? Who is using the documented PnP API? Is svelte kit using that API?

The tests for import-meta-resolve pass with this fix, so I don't see the downside, since import-meta-resolve is broken when using yarn berry presently.

The problem is that this package is supposed to ponyfill import.meta.resolve, but that patch does not do that. So it would introducing broken code into import-meta-resolve. Which I then need to fix. The fix is a month of work.

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.

2 participants