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

Revert "Updated README Link import" #311

Merged
merged 2 commits into from
Oct 18, 2019

Conversation

Alexendoo
Copy link
Collaborator

This reverts commit e195068.

activeClassName requires Link to be imported from match.js

This reverts commit e195068.

activeClassName requires Link to be imported from match.js
@pmkroeker
Copy link
Contributor

When I was testing I found that I had to import Link from root. Gave me errors trying to import from match.js.

@Alexendoo
Copy link
Collaborator Author

What errors did you run into? The link exported by the root and match do different things, only the one in match supports the activeClassName thing displayed in the README

@pmkroeker
Copy link
Contributor

I was getting match does not export Link, which after looking at the code seems strange. Maybe it was an unrelated issue. Next time I'm using it I'll see what happens.

@marvinhagemeister
Copy link
Member

Having two components with the same name do different things is confusing. Maybe we should revisit that decision and combine them or export them as different names all together?

@Alexendoo
Copy link
Collaborator Author

I think that would be a good idea. Perhaps we could bring in match.js's contents to index.js, with a re-export so it's not a breaking change.

@JLWalsh
Copy link

JLWalsh commented Aug 1, 2019

I 100% support this PR, I was really confused and had to dive in the code to figure out that there's a link exported by the router and a link exported by the match.

@developit
Copy link
Member

If we can golf the size of match.js down, merging it into index.js is fine by me. I just didn't want to pay the extra 500b.

@developit developit merged commit 113ea77 into preactjs:master Oct 18, 2019
@Alexendoo Alexendoo deleted the README-import branch October 19, 2019 11: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.

5 participants