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

Add GitHub Action CI file #4

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add GitHub Action CI file #4

wants to merge 1 commit into from

Conversation

grimmer0125
Copy link
Collaborator

@grimmer0125 grimmer0125 commented Nov 21, 2021

ref: #3

todo:

  • create CI yaml and most of the basic steps
  • add building Pyodide JS part (make build/pyodide.js or make src/js/pyproxy.gen.js) since pyodide.js needs the generated pyproxy.gen.js for import "./pyproxy.gen.js".

@grimmer0125 grimmer0125 marked this pull request as ready for review November 21, 2021 15:53
@grimmer0125 grimmer0125 marked this pull request as draft November 21, 2021 15:54
@grimmer0125 grimmer0125 force-pushed the add_github_ci branch 3 times, most recently from c6cd7f4 to 9d11618 Compare November 21, 2021 16:15
@grimmer0125
Copy link
Collaborator Author

grimmer0125 commented Nov 21, 2021

@rth could you help me to change the default branch to main?

Also, do you think installing emcc toolchain to generate pyproxy.gen.js in GitHub CI env. is a doable way? If I execute make build/pyodide.js or make src/js/pyproxy.gen.js and it will hang on # We can't input pyproxy.js directly because CC will be unhappy about the file and I guess it is due to missing the toolchain.

--
BTW, in the makefile

build/pyodide.js: src/js/*.js src/js/pyproxy.gen.js node_modules/.installed
	npx typescript --project src/js

npx typescript seems to be incorrect and npx tsx is the correct one.

--
Also, I saw npx rollup -c src/js/rollup.config.js outputs ./src/js/pyodide.js → build//pyodide.mjs...
I do not see pyodide.mjs** file in the published Pyodide npm, does it mean that some Rollup or Publishing has something wrong?

@rth
Copy link
Member

rth commented Nov 21, 2021

could you help me to change the default branch to main?

Sure, done. The issue was that there were both main and master (default) branches that where identical. So I deleted main then [renamed master to main](could you help me to change the default branch to main?). You just need to make the change locally as well afterwards,

image

Another alternative for future PRs, just fork this repo to you account (which is a better in any case) and the branches will be correct

@rth
Copy link
Member

rth commented Nov 21, 2021

npx typescript seems to be incorrect and npx tsx is the correct one.

Feel free to open a PR of you think so.

Also, do you think installing emcc toolchain to generate pyproxy.gen.js in GitHub CI env. is a doable way?

I think a better way would be to build and deploy the NPM package for each commit on main, that way you could install it from say https://cdn.jsdelivr.com/pyodide/dev/latest/js/pyodide.tar.gz (or some other file formal), or if there is some other location we could deploy to, without having to depend on the build system. I'm not sure what's the best way to deploy a built NPM package elsewhere than to NPM: would npm pack work ?

I do not see pyodide.mjs** file in the published Pyodide npm, does it mean that some Rollup or Publishing has something wrong?

As far as I understand that's the bundler output: are those usually included in the uploaded npm package? If you find that it should be included, of course, feel free to open a PR.

@grimmer0125
Copy link
Collaborator Author

grimmer0125 commented Nov 22, 2021

Another alternative for future PRs, just fork this repo to you account (which is a better in any case) and the branches will be correct.
Got it.

I think a better way would be to build and deploy the NPM package for each commit on main, that way you could install it from say https://cdn.jsdelivr.com/pyodide/dev/latest/js/pyodide.tar.gz (or some other file formal), or if there is some other location we could deploy to, without having to depend on the build system. I'm not sure what's the best way to deploy a built NPM package elsewhere than to NPM: would npm pack work ?

It is a good idea. I did not use npm pack but I would prefer to install a development npm package (exact same format as the published one). Either publishing to NPM with prelease tags or using GitHub NPM packages to host might be OK.

I do not see pyodide.mjs** file in the published Pyodide npm, does it mean that some Rollup or Publishing has something wrong?

As far as I understand that's the bundler output: are those usually included in the uploaded npm package? If you find that it should be included, of course, feel free to open a PR.

It depends on the usages. From my recent study, I saw some bunder will out .mjs file as ESModule, apart from CommonJS module. The modern way is to contain two output folders when publishing.
. This is related to pyodide/pyodide#1949 (comment)

a) Some use Node.js conditional export (in package.json) with .mjs

{
  "type": "module",
  "main": "./index.cjs",
  "exports": {
    "import": "./index.mjs",  <- ES6 import module 
    "require": "./index.cjs"
  }
}

I think this is for the users who do not set "type": "module" (even without a bunder/babel) and they rely on file extensions to tell whether a used module is ESModule.

As I remember, neither "Parcel/Rollup" uses conditional export by default.

b) Some use "module" filed in package.json. They are used by parcel/rollup etc.

{
   // usually this file and its used files are built for CommonJS `require` syntax)
  "main": "build/main/index.js"

  // (internally ES6 import syntax), maybe some bundler use `index.mjs` here
  "module": "build/module/index.js",  

But for Rollup case, I'm not sure whether the .mjs is the intermediate file or the final one, need to figure it out.

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.

2 participants