Opened 3 months ago

Closed 3 months ago

#30933 closed defect (fixed)

Get rid of CoffeeScript from the JS proxy

Reported by: arlolra Owned by: arlolra
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords:
Cc: arlolra, cohosh, phw, dcf Actual Points:
Parent ID: Points:
Reviewer: dcf Sponsor:

Description

The semantics are a barrier to contribution and good parts are all upstreamed in ES2015 and beyond.

Child Tickets

Change History (12)

comment:1 Changed 3 months ago by dcf

I'm in agreement with this idea.

One proposal for how to do it is: take the current CoffeeScript-generated JS, run it through a pretty-printer, and commit. Then do a second manual commit for formatting and refactoring to remove autogeneration-isms. This way, by looking at the diffs, we can be reasonably sure that the port from CoffeeScript to JS doesn't change any semantics.

comment:2 Changed 3 months ago by arlolra

There's also the build and test suite runner to think about, which is currently using a Cakefile,
https://gitweb.torproject.org/pluggable-transports/snowflake.git/tree/proxy/Cakefile

comment:3 Changed 3 months ago by arlolra

Owner: set to arlolra
Status: newassigned

comment:4 Changed 3 months ago by arlolra

Here's a WIP branch that mostly gets this done,
https://github.com/keroserene/snowflake/commits/coffee

Still a few loose ends to tie up but some early review would go a long way since rebasing this is going to be nightmare if we make any significant changes.

Thanks

comment:5 Changed 3 months ago by arlolra

Status: assignedneeds_review

comment:6 Changed 3 months ago by arlolra

Still a few loose ends to tie up

Ok, taking this out of WIP

comment:7 Changed 3 months ago by dcf

Reviewer: dcf

comment:8 Changed 3 months ago by dcf

Status: needs_reviewneeds_information

Great job on this. You covered some details that I would have missed. I looked at 4b97c9d524268f5e1242d0e9b537a4dc6486c01f. Just a couple of questions.

  1. b5934883d4 "Don't overwrite global location": The commit also adds checks for snowflake !== null. Are those related to the location change, or could they be split out?
  2. d1af00da67 "Don't specify port for stun server": How was this working before? Was there a hardcoded default inside libwebrtc?

I did the CoffeeScript rebuild step myself starting from 60fe1ae18b^ and got the same output as your 60fe1ae18b.

In order to download the newer version of CoffeeScript needed, I had to modify package.json. It might be worth sneaking in a commit to this effect, as documentation in the log.

-    "coffee-script": "^1.12.2",
+    "coffeescript": "^2",

comment:9 in reply to:  8 Changed 3 months ago by arlolra

  1. b5934883d4 "Don't overwrite global location": The commit also adds checks for snowflake !== null. Are those related to the location change, or could they be split out?

What was happening was the location bug would throw before window.init initialized snowflake and so window.unload would be called with it still as null. I can split them apart if you prefer since they're only related in that the one surfaced the other.

  1. d1af00da67 "Don't specify port for stun server": How was this working before? Was there a hardcoded default inside libwebrtc?

It wasn't working before because, prior to 7f5cd81, it wasn't being set at all. Fixing that "broke" things.

For what it's worth though, I don't think removing the port actually fixes anything. It probably makes the url invalid in some way that it falls back to the default you alluded to in libwebrtc.

My investigation sort of ended when discovering it was that commit that was preventing this work from going through. We're going to look at it in more detail in comment:10:ticket:31100

I did the CoffeeScript rebuild step myself starting from 60fe1ae18b^ and got the same output as your 60fe1ae18b.

In order to download the newer version of CoffeeScript needed, I had to modify package.json. It might be worth sneaking in a commit to this effect, as documentation in the log.

-    "coffee-script": "^1.12.2",
+    "coffeescript": "^2",

Sure, the commit message for 60fe1ae does mention this though.

comment:10 Changed 3 months ago by arlolra

Status: needs_informationneeds_review

I looked at ​4b97c9d524268f5e1242d0e9b537a4dc6486c01f. Just a couple of questions.

Here's a newly rebased branch with the changes you requested,
https://github.com/keroserene/snowflake/commits/c2

comment:11 Changed 3 months ago by dcf

Status: needs_reviewmerge_ready

All good.

Note: See TracTickets for help on using tickets.