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

include unpkg (or main) when caching #157

Closed
wants to merge 3 commits into from

Conversation

jgravois
Copy link
Contributor

attempt to resolve #156

disclaimer: 🚒 this patch is 100% untested! 🚒

attempt to resolve mjackson#156 

disclaimer: 🚒 this patch is 100% untested! 🚒
@@ -7,6 +7,7 @@ function cleanPackageConfig(packageConfig) {
return {
name: packageConfig.name,
version: packageConfig.version,
main: packageConfig.unpkg || packageConfig.main,

Choose a reason for hiding this comment

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

Good idea! (if this is what caused the issue, 😄 I haven't tested either!)

there's already quite a bit of logic in filenameRedirect about these things. I think it might be better to directly map packageConfig.main, packageConfig.module, packageConfig.browser, packageConfig.unpkg to this object

unpkg: packageConfig.unpkg,
main: packageConfig.main,
module: packageConfig.module,
"js:next": packageConfig["js:next"],

Choose a reason for hiding this comment

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

this is awesome, great catch on adding the js:next as well. I'd anticipate having to replace the double quotes with single quotes, though; it looks like there was recently a big refactor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. addressed in 0d5f0dc

@feildmaster
Copy link

How has this patch for a breaking bug not been pulled yet? Does mjackson not realize his website is broken?

@jgravois
Copy link
Contributor Author

jgravois commented Dec 20, 2018

How has this patch for a breaking bug not been pulled yet?

this is far from a showstopper. if you need the problem fixed immediately, update your own links to reference the explicit files you need to load.

@feildmaster
Copy link

feildmaster commented Dec 20, 2018

It breaks a feature that has been advertised and known to work until now.

Now that you mention updating reference links, if I had still depended on this feature for some of my projects, there would have been no way for me to get people to update other than bothering them by word of mouth to reinstall it manually. Luckily I have a separate file for version detection, but I still believe it wont download the file correctly until this bug is fixed. But this is my own fault for depending on 3rd party features rather than pointing directly to the files. Luckily @latest still works.

@antoniandre
Copy link

It breaks a feature that has been advertised and known to work until now.

Now that you mention updating reference links, if I had still depended on this feature for some of my projects, there would have been no way for me to get people to update other than bothering them by word of mouth to reinstall it manually. Luckily I have a separate file for version detection, but I still believe it wont download the file correctly until this bug is fixed. But this is my own fault for depending on 3rd party features rather than pointing directly to the files. Luckily @latest still works?

@latest does not redirect to the right file, but if you point to the right file with @latest it will do the job.

@mjackson
Copy link
Owner

Thanks for the PR, @jgravois. Sorry I didn't see it before now. I already made a change that supersedes this one in 961ea49, so we won't need this anymore. But I will definitely remember to do this if we take this route in the future :)

@mjackson mjackson closed this Dec 21, 2018
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.

Incorrect redirection leading to not found js
5 participants