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

[TypeScript Reference] Attempt to sync netflix typescript changes into working branch (NO MERGE) #292

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MSeal
Copy link
Member

@MSeal MSeal commented May 20, 2020

I have not made this branch run yet, just doing a dump of changes made on the netflix fork internally somewhere it can be evaluated for use in commuter open source. The code changed pretty dramatically in how it handled typescript assets and organized some things so I didn't bother with trying to merge git histories. Likely many of the changes need to use the current master code instead, but we can discuss what to do with the branch at our next sync meeting.

@MSeal MSeal requested review from rgbkrk and captainsafia May 20, 2020 00:46
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@@ -0,0 +1,101 @@
// The config object that gets passed around to instantiate the server
Copy link
Member

Choose a reason for hiding this comment

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

No major changes here. Just some stronger typings and some different ways for generating the config.

@@ -0,0 +1,256 @@
import express = require("express");
Copy link
Member

Choose a reason for hiding this comment

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

This is a new edition that adds support for reading raw files from an S3. Not sure if migrating this is totally necessary.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Took a skim at this. I don't think we should merge it right away but we can use some of this code when migrating the codebase to TypeScript.

@rgbkrk
Copy link
Member

rgbkrk commented Jun 7, 2020

I don't think we should merge it right away but we can use some of this code when migrating the codebase to TypeScript.

Totally agree! This code is also likely more out of date than if we switched things over again now.

@MSeal MSeal changed the title [WIP] Attempt to sync netflix typescript changes into working branch [TypeScript Reference] Attempt to sync netflix typescript changes into working branch (NO MERGE) Jun 8, 2020
@MSeal
Copy link
Member Author

MSeal commented Jun 8, 2020

I changed the title to better reflect the state of the code changes. I think we should leave this open while doing typescript improvments as a references?

@captainsafia
Copy link
Member

Yep. This comment has more info about our action plan.

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.

3 participants