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

Optimization of averaging code; set up & test the SW JS library #290

Merged
merged 3 commits into from
Jan 14, 2022

Conversation

Georjane
Copy link
Collaborator

@Georjane Georjane commented Jan 13, 2022

Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!

  • tests pass -- see README.md for how to run them
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request are descriptively named
  • if possible, multiple commits squashed if they're smaller changes
  • reviewed/confirmed/tested by another contributor or maintainer

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

Please alert developers on [email protected] when your request is ready or if you need assistance.

Thanks!

@welcome
Copy link

welcome bot commented Jan 13, 2022

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄

@gitpod-io
Copy link

gitpod-io bot commented Jan 13, 2022

@Georjane Georjane self-assigned this Jan 13, 2022
@jywarren
Copy link
Member

@Georjane that looks perfect. Now you need to a) change the version number in package.json, and b) run npm install to generate a new package-lock.json file. Then commit those files to the same PR!

@jywarren
Copy link
Member

Once we do that all and merge it, we can run the steps to publish to the NPM registry, which would be npm publish -- however you have to have permissions to do that. @Georjane can you create an npmjs.org username if you haven't already and tell me so I can add you to the list of people who can publish? Thanks!

@jywarren
Copy link
Member

And just checking, sometimes it's a convention to merge to a stable branch and publish from there, but it looks like in this repository we have just been publishing from the main branch so we should be OK to do that after merging this PR. Let's do be sure we've compiled the code into the /dist/ folders though... the README should say but I think it's either npm run build or grunt build.

@jywarren
Copy link
Member

Typically npm modules can just be left uncompiled if they're going to be used by another node-based environment. But since ours is designed to run in the browser, which can't do things like require other JS files using require('modulename'), we have to compile it using Browserify or something similar like Webpack. I forget which we're using here, but it's all taken care of in the build script!

@Georjane
Copy link
Collaborator Author

npmjs.org

Once we do that all and merge it, we can run the steps to publish to the NPM registry, which would be npm publish -- however you have to have permissions to do that. @Georjane can you create an npmjs.org username if you haven't already and tell me so I can add you to the list of people who can publish? Thanks!

Hi @jywarren
https://www.npmjs.com/~georjane this is my npm profile which I created with username georjane

@Georjane
Copy link
Collaborator Author

@jywarren @Tlazypanda
I made the above changes required. Please can this PR be reviewed again? Thank you

@@ -1,6 +1,6 @@
{
"name": "spectral-workbench",
"version": "0.2.2",
"version": "0.2.3",
Copy link
Member

Choose a reason for hiding this comment

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

Nice!!

@jywarren
Copy link
Member

This looks great. Let's merge and then I'll add you, then you should be able to check out your local main branch, run git pull origin main and get the latest main branch, including this PR.

Then you should be able to run npm login and npm publish!

And in a few hours it'll appear as a dependabot PR on the rails app repo. Or we can manually trigger it!

@jywarren jywarren merged commit 2d70797 into main Jan 14, 2022
@welcome
Copy link

welcome bot commented Jan 14, 2022

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to https://spectralworkbench.org in the next few days.
In the meantime, can you tell us your Twitter handle so we can thank you properly?
Now that you've completed this, you can help someone else take their first step!
See: Public Lab's coding community!

@Georjane
Copy link
Collaborator Author

npm publish

This looks great. Let's merge and then I'll add you, then you should be able to check out your local main branch, run git pull origin main and get the latest main branch, including this PR.

Then you should be able to run npm login and npm publish!

And in a few hours it'll appear as a dependabot PR on the rails app repo. Or we can manually trigger it!

Thanks @jywarren
will run the npm publish command once I have the permissions. I have checked into the main branch and pulled.

@jywarren
Copy link
Member

OK, I added you! Once you publish, you should momentarily see the version at https://www.npmjs.com/package/spectral-workbench/ go up by 0.0.1!

@jywarren
Copy link
Member

Hooray!
image

@jywarren
Copy link
Member

So triggering dependabot is a bit obsure. But you can do it from this page: https://github.com/publiclab/spectral-workbench/network/updates

@jywarren
Copy link
Member

Oh, odd - SWB doesn't have a javascript dependabot setting. I wonder why not? Usually it'll note yarn.lock or package.json, here I see only the Gemfile for Ruby dependencies...

@jywarren
Copy link
Member

aha- sorry, actually it was misconfigured - we haven't been getting any JS updates at all! 😱

Not a huge deal, but you can see here, all are Ruby: https://github.com/publiclab/spectral-workbench/issues?q=label%3Adependencies+is%3Aclosed

The difference is we only have bundler (for ruby gems) in the config file: https://github.com/publiclab/spectral-workbench/blob/4592de0b9471e49fc8c1146d9d7e296df5add9ef/.github/dependabot.yml#L3

vs. also including NPM as we do in plots2: https://github.com/publiclab/plots2/blob/5a4937bbc68d6348e0a7f9df05ae0d182c4539a7/.github/dependabot.yml#L46

I'm not sure how this happened but I'll change it now.

@jywarren
Copy link
Member

Great - ok done and it's checking now. It might find a bunch of other JS ones that it prioritizes first, i'm not sure how it chooses which.

@jywarren
Copy link
Member

Yes and they're starting to appear here: https://github.com/publiclab/spectral-workbench/pulls

What we'll want to do is try installing it locally in a copy of the Rails app, to see if we can pull it in and if anything breaks. Ideally we can test out the thing you tweaked, but not a huge deal if not since it's such a small thing.

Then once we can get the dependabot PR to pass tests (it can happen that we need to make fixes to the JS library and re-release a bugfix to our release, as 0.2.4, but probably not in this case), we merge it, then it'll get auto-published to https://stable.spectralworkbench.org/ and we can test it out there!

There are a lot of steps here, but that's to help ensure things work properly along the way.

@jywarren
Copy link
Member

I'm merging some so we get through all the backlog of JS dependabot updates. Many are for simple things or very small updates so we can be pretty sure.

@jywarren
Copy link
Member

Well, I'm not sure why dependabot isn't picking it up. It's very low on the list alphabetically and in the listing at https://github.com/publiclab/spectral-workbench/blob/525dfe41e5d0259054c7ac7116caf6d3d4bbfa2c/package.json#L24

...so maybe that's it. We can just do it manually this time, and maybe next time dependabot will have 'caught up'. To do it manually you can check out the main branch of spectral-workbench (the rails app) and then make a feature branch from it, then, update the package.json file to:

"spectral-workbench": "^0.2.3"

That way it'll force it to use at least the newest version. Then run yarn install - and commit the yarn.lock, package.json files in a new PR. Then we can try it out a bit and then merge from there!

@jywarren
Copy link
Member

There may be a rate limiting issue with dependabot so it's stopped fetching new version... i'm not totally sure. Sorry about this, it's a little more work than normal since this has not been done in so long!!

@jywarren
Copy link
Member

Retriggered Dependabot now, fingers 🤞

@jywarren
Copy link
Member

I decided to open this manually in publiclab/spectral-workbench#865 -- will move there!

@jywarren jywarren changed the title Set up and Test the SW JavaScript library following installation steps in the Readme Optimization of averaging code; set up & test the SW JS library Jan 18, 2022
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