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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring project to modern day Rust WASM #15

Merged
merged 19 commits into from Apr 7, 2023
Merged

Conversation

ilyvion
Copy link
Contributor

@ilyvion ilyvion commented Apr 2, 2023

Hey @lpil. 馃憢

This regex tester tool of yours is used by a lot of people, but I couldn't help but notice it was just sitting there, sad and dilapidated, and I thought I'd help bring some life back into it!

In the process made some executive decisions that I'd like to run by you before I turn this into a non-draft pull request, so we're on the same page before anything happens. These are the changes I decided to go ahead and make (and of course you can also just look at the commits/file changes if that's more your style):

  • I removed build artifacts from the repo. I don't generally think it's a good idea to put "binaries" into a Git repo; they can't be dealt with as deltas by git, and so your repo will grow in size each time by as much as the size of each new artifact. As a replacement for this, I set up a GitHub Actions workflow to automatically build and deploy the artifact as needed. You can see these actions in... action on my fork as well as the result of this automatic deployment under my GitHub pages for said fork. (Yes, that is running the most up-to-date version that I've been working on.)
  • Which brings me to my next point. In order for that GH Actions workflow to deploy properly to your site, I'd need to know how you've configured your deployment/GH Pages, or you have to change yours to match mine. The latter is probably more appropriate. While I assume you've set yours to deploy from your master branch from under /docs, it makes more sense for automatic deployment for it to happen from the gh-pages branch at /. So that's the approach I recommend you take, should you choose to move forward with this PR.
  • I moved away from stdweb, which hasn't seen an update in over 3 years, and over to wasm-bindgen and web-sys, both actively maintained and the currently most appropriate crates for Rust/WASM<->JS interop, and replaced cargo-web, which again, hasn't seen an update in over 3 years, with trunk, a modern WASM application bundler that makes working with Rust-for-WASM a breeze.
  • I set a fairly aggressive lint policy (see the first ~130 lines of main.rs) and fixed anything about the code that was in violation of said lints.
  • I updated regex to the latest version. Probably my least controversial decision tbh.
  • I hid the "Split on newlines?" checkbox because as far as I can tell, it doesn't actually have any logic associated with it.

Well, there you go; if you have any feedback, I'm very open to making adjustments.

While I'm at it, I'll also say that I'm willing to take over stewardship of this project, should you wish not to take on/keep the maintenance burden of the project; I'd even go so far as to take on the hosting burden (I mean, I'd still use GH pages for it, but I'd be willing to use a domain name under my control if you'd like.)

@ilyvion ilyvion closed this Apr 6, 2023
@lpil
Copy link
Owner

lpil commented Apr 6, 2023

Hey @alexschrod , sorry only just saw this. Looks great!

Was the CNAME file intentionally removed? I think we'd still need that, unless github pages has changed without my realising. :)

@lpil lpil reopened this Apr 6, 2023
@lpil
Copy link
Owner

lpil commented Apr 6, 2023

Oh, it looks like some other commits have since been added to the branch. Shall I undo those?

@ilyvion
Copy link
Contributor Author

ilyvion commented Apr 6, 2023

Yeah, sorry, with your lack of response, I was in the middle of "going rogue" and decided to take things into my own hands, as I'd noticed you'd been fairly active on GitHub for days after I opened this PR, so I thought you weren't interested since you didn't respond. I can undo some of that if you're open to it after all. 馃槃

@ilyvion
Copy link
Contributor Author

ilyvion commented Apr 6, 2023

As for the CNAME, it should be added during the GH Action run. I can set that up, too.

I was planning for you to move over to the new model of GH pages since it looks like you're on the very old one from back when they could only exist in a /docs folder under master.

@ilyvion
Copy link
Contributor Author

ilyvion commented Apr 6, 2023

With this new change, I'll fix things up and let you know when it's ready for review again. 馃槃

ilyvion and others added 8 commits April 6, 2023 16:44
No copyright assignment agreement exists in this repo; thus each individual contribution holds the copyright over each piece of their own code, so the copyright statement should reflect this.
Also, the license should not be edit but be presented as is, verbatim. The notice example is a template for how to show it in a program, not one to be edited in the license.
(cherry picked from commit 7e209b5)

# Conflicts:
#	src/main.rs
(cherry picked from commit c55e34e)
(cherry picked from commit fff58b8)

# Conflicts:
#	src/main.rs
which is a lie, given that the code is AGPL licensed, which "gives up" a lot of the rights of "default" copyright.
@ilyvion
Copy link
Contributor Author

ilyvion commented Apr 6, 2023

Alright @lpil. This PR has gotten a bit bigger than originally intended since I "moved on" when I thought you weren't interested and started changing some things and adding some features that I wanted to add to my updated version. In addition to what I stated in the OP, the following things are now also part of this PR:

  • Added the ability to use fancy-regex instead of regex when testing regular expressions. This only added about 100 KiB to the generated WASM file and felt like it was worth it
  • While the project was created by you, it has contributions from multiple people, and so I felt it was appropriate to say as much, especially considering that the project does not have a copyright assignment agreement, so copyrights of the contributions are held by each individual. As such I removed your name from the copyright statement and made it refer to the contributors page instead.
  • Imported some commits from a fork by @ArpegiusWhooves because I thought they were valuable contributions
  • Removed the phrase "All Rights Reserved" from the copyright statement because it's not true when using a license like the AGPL.

Alright. As for your confusion around the CNAME file, here's what you need to do. I think it might be best to do these bits BEFORE you accept the PR:

  • In order for the build to be deployed successfully, the GITHUB_TOKEN must be given write permissions to the repo, otherwise it can't push to the gh-pages branch. The CI.yml file should be simple enough to vet to ensure that I'm not up to anything mischievous. Instructions on how to enable this write permission.
  • You should go to your repo's Actions secrets and variables page and click "New repository variable," name it "CNAME" and set it to "rustexp.lpil.uk" without the quotes. This will be what goes into the deployed CNAME file that you were concerned with before.

Alright! Once you've done this and AFTER you've accepted this PR and the CI has done its build and deployment successfully, you'll then want to go to your repo's pages configuration, and make changes so it looks like this:
image

Alright. That should be everything. Let me know if you have any questions, concerns, comments or any other kind of feedback. I (or you) can set this PR to not be a draft and you can approve it once you're satisfied you're ready to accept it.

@lpil
Copy link
Owner

lpil commented Apr 7, 2023

Wonderful! Thank you very much for all this work, and your thorough write up. Super helpful 馃挏

I'd noticed you'd been fairly active on GitHub for days after I opened this PR, so I thought you weren't interested since you didn't respond. I can undo some of that if you're open to it after all. 馃槃

Sorry, my inbox is just very busy 馃槄

Removed the phrase "All Rights Reserved" from the copyright statement because it's not true when using a license like the AGPL.

I don't think that this isn't true. "all rights reserved" means the copyright holders have not waived their rights, it does not conflict with a licence which gives others certain rights also, rather AGPL relies upon copyright being held.
Having said that, it doesn't actually do anything any more. One don't need to write that to state that they retain copyright, so no reason to have it really. Cultural habit I suppose :)

@lpil lpil marked this pull request as ready for review April 7, 2023 11:20
@lpil lpil merged commit d50e517 into lpil:master Apr 7, 2023
@ilyvion
Copy link
Contributor Author

ilyvion commented Apr 7, 2023

Noice. 馃槃 Do you mind if I post about this update/upgrade in various places, like the Rust subreddit or the Rust users forum? Or would you like to do so yourself, perhaps?

@lpil
Copy link
Owner

lpil commented Apr 8, 2023

What a cracking idea. Please do go ahead @alexschrod ! I look forward to reading it. :)

@ilyvion
Copy link
Contributor Author

ilyvion commented Apr 9, 2023

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.

None yet

3 participants