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

chore(client): move the client to its own subfolder #1838

Closed
wants to merge 6 commits into from

Conversation

daniellacosse
Copy link
Contributor

@daniellacosse daniellacosse commented Feb 12, 2024

I was able to successfully move the client into its own subfolder:

Screenshot 2024-02-12 at 1 49 21 PM

TODO:

  • should i move build into the root? or try to mess with ROOT_DIR?
  • basic documentation
  • move third_party into the root (need help from @jyyi1)
  • unbreak it... again
    • electron
    • windows (it can't find the env.nsh)
    • android working in the CI (dialer.Dial undefined??)
    • maccatalyst working in the CI

.replace(/</g, '\\<')
.replace(/>/g, '\\>;')
.replace(/&/g, '\\&');
return str.replace(/"/g, '\\"').replace(/'/g, "\\'").replace(/</g, '\\<').replace(/>/g, '\\>;').replace(/&/g, '\\&');

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This does not escape backslash characters in the input.
.replace(/</g, '\\<')
.replace(/>/g, '\\>;')
.replace(/&/g, '\\&');
return str.replace(/"/g, '\\"').replace(/'/g, "\\'").replace(/</g, '\\<').replace(/>/g, '\\>;').replace(/&/g, '\\&');

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This does not escape backslash characters in the input.
.replace(/</g, '\\<')
.replace(/>/g, '\\>;')
.replace(/&/g, '\\&');
return str.replace(/"/g, '\\"').replace(/'/g, "\\'").replace(/</g, '\\<').replace(/>/g, '\\>;').replace(/&/g, '\\&');

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This does not escape backslash characters in the input.
.replace(/</g, '\\<')
.replace(/>/g, '\\>;')
.replace(/&/g, '\\&');
return str.replace(/"/g, '\\"').replace(/'/g, "\\'").replace(/</g, '\\<').replace(/>/g, '\\>;').replace(/&/g, '\\&');

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This does not escape backslash characters in the input.
@daniellacosse daniellacosse added client code health Improvements to code or maintainability cleanup needs test Pull requests that require tests labels Feb 12, 2024
@daniellacosse daniellacosse changed the title proposal(client): move the client to its own subfolder and rename the repository to outline-apps proposal(client): move the client to its own subfolder Feb 21, 2024
@daniellacosse daniellacosse marked this pull request as ready for review February 21, 2024 21:05
@daniellacosse daniellacosse requested review from fortuna and a team as code owners February 21, 2024 21:05
@daniellacosse daniellacosse changed the title proposal(client): move the client to its own subfolder chore(client): move the client to its own subfolder Feb 21, 2024
Copy link
Collaborator

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

A number of tests are still failing, so I think there's still stuff to figure out.

client/third_party/jsign/.gitignore Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
client/README.md Outdated Show resolved Hide resolved
@daniellacosse daniellacosse marked this pull request as draft February 22, 2024 02:51
@daniellacosse
Copy link
Contributor Author

daniellacosse commented Feb 22, 2024

A number of tests are still failing, so I think there's still stuff to figure out.

Sorry I prematurely took this out of draft. Let me put it back into draft mode.

@daniellacosse daniellacosse removed needs test Pull requests that require tests needs discussion labels Feb 22, 2024
@daniellacosse daniellacosse force-pushed the daniellacosse/contain_cordova branch 2 times, most recently from a982523 to 3eb6a85 Compare February 28, 2024 01:19
Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

For the Windows build failure, we just need to specify the projectDir when calling electron-builder (currently it's D:\a\outline-apps\outline-apps, but it should be D:\a\outline-apps\outline-apps\client because env.nsh will be generated there), similar to what we did in manager.

@daniellacosse daniellacosse force-pushed the daniellacosse/contain_cordova branch from 767dc3f to 92aaae9 Compare March 5, 2024 08:52
@daniellacosse
Copy link
Contributor Author

@jyyi1 I've updated the SDK - didn't you say some naming changed? What should I fix

@jyyi1
Copy link
Contributor

jyyi1 commented Mar 7, 2024

@jyyi1 I've updated the SDK - didn't you say some naming changed? What should I fix

Basically anytime you see an error: dialer.Dial undefined (type transport.StreamDialer has no field or method Dial), just rename the Dial in the source code to DialStream (if the error message mentions StreamDialer) or DialPacket (if the error message mentions PacketDialer).

@daniellacosse daniellacosse force-pushed the daniellacosse/contain_cordova branch 2 times, most recently from 896396c to 3bafef8 Compare March 13, 2024 14:42
@github-actions github-actions bot added size/XL and removed size/XXL labels Mar 13, 2024
@daniellacosse daniellacosse force-pushed the daniellacosse/contain_cordova branch from 6894a19 to 9229b6d Compare March 14, 2024 04:17
@daniellacosse
Copy link
Contributor Author

daniellacosse commented Mar 14, 2024

For the Windows build failure, we just need to specify the projectDir when calling electron-builder (currently it's D:\a\outline-apps\outline-apps, but it should be D:\a\outline-apps\outline-apps\client because env.nsh will be generated there), similar to what we did in manager.

I'm not really sure where it's looking for the env.nsh and the surrounding files here don't really give me a clue:

I've tried copying it to several directories and it still fails to find it. Any ideas?

@@ -50,7 +50,7 @@ if [[ -z "${WEBPACK_MODE:-}" ]]; then
fi

# Build the Web App.
node src/build/run_action.mjs server_manager/web_app/build
node client/src/build/run_action.mjs server_manager/web_app/build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the Manager referring to the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to hoist the build folder into the root, I'm planning on doing that next

@fortuna
Copy link
Collaborator

fortuna commented Mar 14, 2024

@daniellacosse can we perhaps only migrate Cordova in this PR so we can move forward quickly?

@daniellacosse
Copy link
Contributor Author

@daniellacosse can we perhaps only migrate Cordova in this PR so we can move forward quickly?

Yes!! I asked in the chat if we were okay with that but I guess it got lost. I can try to make that work once I get back.

@daniellacosse daniellacosse force-pushed the daniellacosse/contain_cordova branch from b3961a4 to 9f6190a Compare April 1, 2024 18:14
sbruens and others added 5 commits April 1, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup client code health Improvements to code or maintainability size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants