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 single asterisk replacement in reverse-exports #1676

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Conversation

mansona
Copy link
Member

@mansona mansona commented Nov 20, 2023

I took the reverse-exports package out for its first real-world spin today by combining #1648 #1650 and #1653 into one giant mega branch 💪 ... but it didn't work 😂

I first looked at the error and thought "on no we need to update the blueprint because the exports are wrong" but thinking about it a bit better it looks like our reverse-exports could do with a little bit of love. This is totally expected in the dev process of the package since real-world examples will always beat lab-built ones 👍

@lolmaus if you could take a look at this and get the test passing that would be super 🎉

@lolmaus
Copy link
Collaborator

lolmaus commented Nov 21, 2023

I have found this in the documentation:

* maps expose nested subpaths as it is a string replacement syntax only.

Also, the mention of ** the double asterisk glob in the documentation does not actually allow using it in keys or values!

The property of exports being statically enumerable is maintained with exports patterns since the individual exports for a package can be determined by treating the right hand side target pattern as a ** glob against the list of files within the package.

Thank you for the heads up, I know exactly how to fix it.

@lolmaus
Copy link
Collaborator

lolmaus commented Nov 21, 2023

Fixed now!

Copy link
Member Author

@mansona mansona left a comment

Choose a reason for hiding this comment

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

I can't "request changes" since it's my PR 🙃 one minor thing and one that I would like to fix before we merge 👍

packages/reverse-exports/tests/reverse-exports.test.ts Outdated Show resolved Hide resolved
packages/reverse-exports/tests/reverse-exports.test.ts Outdated Show resolved Hide resolved
@mansona mansona changed the title recreate a bug in reverse-exports package fix single asterisk replacement in reverse-exports Nov 21, 2023
@mansona mansona marked this pull request as ready for review November 21, 2023 11:57
@mansona mansona added the bug Something isn't working label Nov 21, 2023
@lolmaus lolmaus force-pushed the reverse-exports-bug branch from df09d0a to 77be472 Compare November 21, 2023 12:01
Copy link
Member Author

@mansona mansona left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Copy link
Collaborator

@lolmaus lolmaus left a comment

Choose a reason for hiding this comment

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

Great job, @lolmaus! 😂

@mansona mansona merged commit d68fbec into main Nov 21, 2023
200 of 204 checks passed
@mansona mansona deleted the reverse-exports-bug branch November 21, 2023 12:22
@github-actions github-actions bot mentioned this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants