Skip to content

Docs update #767

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

Merged
merged 4 commits into from
Oct 13, 2020
Merged

Docs update #767

merged 4 commits into from
Oct 13, 2020

Conversation

karray
Copy link
Contributor

@karray karray commented Oct 10, 2020

I've added one more example to "Using Pyodide from Javascript".

As this is my first experience contributing open source projects, I would like to ask is it allowed to include an external link to a live demo into the docs?

@stefnotch
Copy link
Contributor

That looks like a nice improvement to the documentation!
Regarding the external demo, I think those are usually to be avoided, simply because they might go down at some point.

@rth
Copy link
Member

rth commented Oct 11, 2020

Thanks @karray and @stefnotch ! Aside for the external example link (that it would indeed be better not include) LGTM.

@karray
Copy link
Contributor Author

karray commented Oct 12, 2020

Thanks for your feedback. I removed the external link and added a couple of new examples.

@karray
Copy link
Contributor Author

karray commented Oct 12, 2020

Why do we need

// set the pyodide files URL (packages.json, pyodide.asm.data etc)
window.languagePluginUrl = 'https://pyodide-cdn2.iodide.io/v0.15.0/full/';

Everything seems to work without this line of code since the baseURL is initialized in pyodide.js

var languagePluginLoader = new Promise((resolve, reject) => {
// ...
  var baseURL = self.languagePluginUrl || 'https://pyodide-cdn2.iodide.io/v0.15.0/full/';
// ...

@rth
Copy link
Member

rth commented Oct 13, 2020

Everything seems to work without this line of code since the baseURL is initialized in pyodide.js

Yes, it will work with the default URL but it will fail when using a locally deployed pyodide server. It's better to include it explicitly so users don't run into hard to debug errors with local deployments.

@rth
Copy link
Member

rth commented Oct 13, 2020

Thanks @karray !

@rth rth merged commit b08e460 into pyodide:master Oct 13, 2020
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.

3 participants