Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

feat: virtual database entries for npm and github #659

Merged
merged 8 commits into from
May 28, 2020

Conversation

pi0
Copy link
Contributor

@pi0 pi0 commented May 20, 2020

The first time I came to the deno.land website I was quite surprised not to be able to resolve my repo.

For the listing, of course, it makes sense to make a PR to database.json but with this addition, any project can still have an elegant URL while keeping the website scalable.

New working URLs:

Preview:

@lucacasonato
Copy link
Member

Ref #406

I am in favor of this change 👍. Don't know about @ry.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @pi0 and sorry for the delay.

My only concern about this is that I'm not sure I want to support this yet forever. I've labeled it as an experimental feature - let's see how it goes.

@ry ry merged commit 930a182 into denoland:master May 28, 2020
@pi0
Copy link
Contributor Author

pi0 commented May 28, 2020

Thanks, @ry for helping to merge. I agree it may not be the best URL pattern for the long term.

@pi0 pi0 deleted the feat/virtual-db-entry branch May 28, 2020 12:50
@srdjan
Copy link

srdjan commented May 28, 2020

nice!

Question (possibly mistyped):

New working URLs:

shouldn't it be:

btw, what is the reason to resolve to the root of the repo?
wouldn't pointing to 'dist' folder, be more proper?

@pi0
Copy link
Contributor Author

pi0 commented May 28, 2020

@srdjan Yes could be simpler but I think it also adds some implications in cost of omitting /dist from URL that: There is always a build step which should update dist and also checked-in into repository. Taking a1 as an example, it is simply using top-level index.ts

PS: Updated PR description for owner:repo 👍 Actual code change seems fine

@pi0
Copy link
Contributor Author

pi0 commented Nov 8, 2020

Just for reference seems this solution is deprecated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants