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

Setup NPM package and use TypeScript #40

Merged
merged 5 commits into from
Apr 4, 2023
Merged

Setup NPM package and use TypeScript #40

merged 5 commits into from
Apr 4, 2023

Conversation

bjoluc
Copy link
Contributor

@bjoluc bjoluc commented Oct 5, 2022

Following up on #38, this sets up an NPM package and – different than originally planned – also sets up TypeScript. I figured it would probably not be a good idea to set up a custom build chain in this repo that – however simple it may be – needs to be maintained, so I opted for the one used by the packages in jsPsych and jspsych-contrib repos, which is readily available via @jspsych/config.
The source file is src/index.ts now, and jspsych-psychophysics.js is generated by the build chain on running npm build. It includes mathjs and numeric for convenience, but not PixiJS (due to its heavy size). Hence, all demos continue working with the current file paths. There is a breaking change however that I couldn't avoid: When using PixiJS, the Pixi script needs to be included before the psychophysics plugin or it won't be available to jsPsychPsychophysics.

This PR adds a lot of development tooling under the hood and I'm happy to explain everything in detail, including some remaining choices – ideally in a face-to-face session. Just let me know if you're interested. Cheers!

@kurokida
Copy link
Owner

kurokida commented Oct 6, 2022

Thank you very much!

I have some questions. Sorry for bothering you, but I'm very new to npm, so I think my questions are elementary.

  • Before merging your PR, I downloaded files from your repo. Is it okay?
  • After downloading the files, I opened the root directory in Node.js command prompt. Then I run npm build command. But the following error messages appeared:

Unknown command: "build"
Did you mean this?
npm run build # run the "build" package script

  • How did you make the package.json file? Did you make it manually?

I hope I could talk with you in a face-to-face session someday, but I'm not good at English conversation. So communication using text is convenient for me at least now.

Best,
Daiichiro

@bjoluc
Copy link
Contributor Author

bjoluc commented Oct 6, 2022

Before merging your PR, I downloaded files from your repo. Is it okay?

Sure thing!

After downloading the files, I opened the root directory in Node.js command prompt. Then I run npm build command.

My bad, sorry! I meant to write npm run build. In case you didn't know: After cloning the repo, you should first run npm install to install all dependencies from package.json at the exact versions locked in package-lock.json. They'll be saved into node_modules then.

How did you make the package.json file? Did you make it manually?

I copied it from jspsych-contrib and modified it by hand. Alternatively npm init is designed to create a package.json from scratch.

So communication using text is convenient for me at least now.

Works for me too, no probs! So just feel free to ask me anything :)

@kurokida
Copy link
Owner

kurokida commented Oct 7, 2022

Thanks to you, I am making some progress!
I could run the npm install command and the required modules were installed in the node_modules folder successfully.

In fact, I tried to convert my plugin file to TypeScript before, but I gave up. You seem to have succeeded in doing that. How did you make the src/index.ts file? I guess you have used some useful tools.

When I add new features to my plugin or fix bugs, will I change the src/index.ts file?

Actually, I tried to run my sample programs, but it failed.
For example, when I run psychophysics-demos/cross.html, I face some error messages. See the attachment file.

Best regards,
Daiichiro

@bjoluc
Copy link
Contributor Author

bjoluc commented Oct 7, 2022

How did you make the src/index.ts file?

I don't know if there are tools to do that; I just copied jspsych-psychophysics.js -> src/index.ts, modified the IIFE into a module, and then went through the file to fix type errors manually. Many of them were simply fixed by installing dependencies, others required some basic typing. The @jspsych/config/tsconfig.json is pretty lax, so it wasn't a large transformation though. You can stricten the rules in your tsconfig.json later on if you wish to. I also updated mathjs to v11 after examining its changelog to read up on breaking changes.

When I add new features to my plugin or fix bugs, will I change the src/index.ts file?

You name it! In order to test the changes with the examples, you'll have to run npm run build every time. There's also npm run build:watch which does this automatically, but unlike npm run build, it doesn't copy dist/index.browser.js to jspsych-psychophysics.js, so the examples wouldn't load the updated version.

Actually, I tried to run my sample programs, but it failed.

I couldn't find any attachments, but I tested all examples locally before creating the PR. Wild guess: Maybe you do not have d9bc08e locally? I had to make one patch change to @jspsych/config which wasn't yet published when I created the PR. If you pull again, make sure to run npm install again to install the new @jspsych/config version from d9bc08e.

@kurokida
Copy link
Owner

kurokida commented Oct 7, 2022

Sorry I forgot to attach the file.

cross_demo_error

Today I cloned your repo, so I think your final commit is included.

I greatly appreciate making the src/index.ts file. In fact, I am unfamiliar with TypeScript. So I would like to learn how to convert the original JS file to a TS file. Let me try this before merging your PR. It might take a long time.

@bjoluc
Copy link
Contributor Author

bjoluc commented Oct 7, 2022

So I would like to learn how to convert the original JS file to a TS file. Let me try this before merging your PR. It might take a long time.

I really appreciate that! Take your time. You may find the jspsych-contrib plugin template's index.ts helpful for the IIFE => module transformation.

EDIT: Turns out I was lying to you about not using any tools: I used Prettier for code formatting in index.ts!

@bjoluc
Copy link
Contributor Author

bjoluc commented Oct 7, 2022

I ran the demos in Firefox only. Running cross.html in Chrome yields the same error for me! It looks like an encoding thing: Line 54696 is mathjs code, namely for (var λ of values) {. Importing jspsych-psychophysics.js via <script src="../jspsych-psychophysics.js" charset="utf-8"></script> fixes the issue. Now, that would be a fairly breaking change. Let me check if there are other alternatives...

EDIT: After looking into this again, the best solution might be to get rid of math.js. It looks like https://github.com/mljs/matrix can be a slim and efficient replacement here which would vastly reduce the bundle size. It might be interesting to see if it can compete with numeric in terms of speed, so maybe numeric could be dropped as well. Regarding math.js, I already tried custom bundling but it somehow didn't decrease the size of jspsych-psychophysics.js much. Maybe replacing math.js is for another PR though. Let me know if you want me to look into that :)

EDIT 2: Couldn't help but try ml-matrix and it is indeed a perfect replacement for math.js in our situation, decreasing the uncompressed bundle size from bloated 2.2M to 352K 🎉 When dropping numeric too, it's only 192K 🚀

@kurokida
Copy link
Owner

Thank you for your valuable comments and commits.
Changing the libraries my plugin depends on would be a somewhat big decision. I have to carefully confirm if there might be any problems with this change.
Anyway, I greatly appreciate your cooperation!

@kurokida
Copy link
Owner

kurokida commented Dec 7, 2022

Thank you very much for your cooperation!
I'm sorry for taking a long time to merge your PR.
As I mentioned before, this is a pretty big change for my plugin, and I have no time to dive into it now.
I will try this next year.

@bjoluc
Copy link
Contributor Author

bjoluc commented Dec 7, 2022

Hey @kurokida! Sorry if updating this raised the pressure on you; it wasn't intended to (I don't depend on this)! Just making sure the PR won't be outdated once you get to it 🙂 Take your time!

@kurokida kurokida merged commit 262665a into kurokida:master Apr 4, 2023
@kurokida
Copy link
Owner

kurokida commented Apr 4, 2023

Thank you very much for your patience. I have merged your PR. It was a good opportunity to learn about npm and TypeScript.

By the way, after merging your PR, two PRs were automatically created.

Do you know if I should merge these two PRs as well?
#42
#41

@bjoluc bjoluc deleted the npm-typescript branch May 31, 2023 09:16
@bjoluc
Copy link
Contributor Author

bjoluc commented May 31, 2023

Sorry for the late reply and congrats for transitioning! 🎉

By the way, after merging your PR, two PRs were automatically created.

Luckily, those are transitive dependencies of @jspsych/config with no security risk for people using your plugin. You can typically ignore those PRs or disable them altogether. We're updating the @jspsych/config dependencies from time to time to fix these.

The next step for you would be to sign up at npmjs.com and run npm login once, followed by npm publish to publish version 3.6.0 to the NPM package registry. Once done, you can also close #38 🙂
For every new release in the future, you update the version in pacakge.json and run npm publish again to publish the new version to NPM too. Let me know if anything is unclear!

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