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

chore: add `jsdelivr` and `unpkg` support #2443

Open
wants to merge 5 commits into
base: master
from
Open

Conversation

@JounQin
Copy link

JounQin commented Sep 30, 2019

As title

JounQin added 2 commits Sep 30, 2019
@yasuf

This comment has been minimized.

Copy link
Collaborator

yasuf commented Nov 4, 2019

I see we have axios already in https://cdnjs.com/libraries/axios ? Also we have main in the package.json already (https://github.com/jsdelivr/jsdelivr#configuring-a-default-file-in-packagejson) which should be supported by jsdelivr and possibly unpkg, the CDNs should adapt to existing package.json files instead of libraries adapting to them I'd say. Let me know if this is good enough or if we should still move forward with this PR

@JounQin

This comment has been minimized.

Copy link
Author

JounQin commented Nov 4, 2019

The main field file is node module, not point to umd or CDN file.

@yasuf
yasuf approved these changes Nov 4, 2019
@yasuf

This comment has been minimized.

Copy link
Collaborator

yasuf commented Nov 10, 2019

@JounQin as per #1110, axios is already on jsdelivr and unpkg somehow, why do we need to add this as well?

@yasuf

This comment has been minimized.

Copy link
Collaborator

yasuf commented Nov 10, 2019

I see https://github.com/jsdelivr/jsdelivr#publishing-packages, so all our versions should be available in jsdelivr as long as we push them to npm

@yasuf

This comment has been minimized.

Copy link
Collaborator

yasuf commented Nov 10, 2019

closing unless I'm missing something, feel free to repoen

@yasuf yasuf closed this Nov 10, 2019
@MartinKolarik

This comment has been minimized.

Copy link

MartinKolarik commented Nov 10, 2019

@yasuf those fields are not required but make it easier to discover and use the correct file. jsDelivr supports main as well but that points to index.js instead of dist/axios.min.js so this PR makes sense.

@JounQin

This comment has been minimized.

Copy link
Author

JounQin commented Nov 10, 2019

。。。

@JounQin

This comment has been minimized.

Copy link
Author

JounQin commented Nov 10, 2019

@yasuf you asked the same question before, and I've answered it like @MartinKolarik 's comment, and then you approved this PR. But now you asked the same question...

@yasuf

This comment has been minimized.

Copy link
Collaborator

yasuf commented Nov 10, 2019

sorry @JounQin, I hadn't seen the jsdelivr documentation before, as I see right now, both jsdelivr and unpkg links work, and yes, the current main has a different purpose.

The links for the CDNs work fine right now, the part I'm missing is why we need these explicitly instead of having those services take the files from npm? If so, should it point to the regular dist/axios.js or the minified version?

Current links:
https://cdn.jsdelivr.net/npm/axios/dist/axios.min.js
https://unpkg.com/axios/dist/axios.min.js

Even the non-minified versions are up

@JounQin

This comment has been minimized.

Copy link
Author

JounQin commented Nov 10, 2019

@yasuf Personally I prefer:

https://cdn.jsdelivr.net/npm/axios
https://unpkg.com/axios

So that I don't have to know the exact location of dist file, and it should benefit users who doesn't use npm/package manager.

Maybe .min.js by default would make more sense.

@MartinKolarik

This comment has been minimized.

Copy link

MartinKolarik commented Nov 11, 2019

@yasuf in case of jsDelivr, that field is used by jsDelivr website/API to indicate which file is the "right one", plus it allows using the shorter link. It should be the minified version (even if you set axios.js, jsDelivr will convert it to axios.min.js)

@yasuf yasuf reopened this Nov 11, 2019
@yasuf

This comment has been minimized.

Copy link
Collaborator

yasuf commented Nov 16, 2019

@JounQin can you update it to be the minified file?

@JounQin

This comment has been minimized.

Copy link
Author

JounQin commented Nov 16, 2019

@yasuf Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.