-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add https to webpack dev server #28422
Conversation
npm has a |
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
npm has a |
npm has a |
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
I tested. I messaged you on slack |
@justinpersaud on web, this tested well However, the desktop build isn't working. I'll test more and provide more details. We also need to edit the CSP of dev OldDot here. I can make a PR for this one tomorrow morning |
Ok, let me know when you're satisfied with this then and I'll remove hold and do the announcement to contributors. |
This is blocking. @justinpersaud did your desktop build worked?
This is not blocking I gonna sleep now but I'll come back to this tomorrow morning |
the desktop build isn't working
|
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinpersaud the PR is ready to be merged, would it be possible for you to post the announcement to the open source channel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do anything for adhoc builds as well?
Long thread here https://expensify.slack.com/archives/C03TQ48KC/p1698343291525409 But we are changing the hostname to |
Retested with Works well. |
@justinpersaud Please see this Slack message |
This PR changes the URL used by the local NewDot dev server. This doesn't affect Ad Hoc builds. We currently don't have a good solution to test Hybrid Web on Ad Hoc builds. Ad Hoc builds will keep working for Chat functionalities. On Ad Hoc builds, you will just see a blank space for the portion where the iframe is supposed to occupy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akinwale confirmed that the PR works with dev.new.expensify.com
could you post a message to the #expensify-open-source channel and send an email to the engineering team about the upcoming changes?
and then we can merge the PR
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Update: Check out the changes to readme if you're running into this: https://github.com/Expensify/App/pull/28422/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5 This PR broke local dev for me:
|
@erquhart Have you performed these steps? https://expensify.slack.com/archives/C01GTK53T8Q/p1698686381655959 |
I did, found that and added a note to the top of my comment for others a bit ago. Thank you! |
🚀 Deployed to staging by https://github.com/justinpersaud in version: 1.3.94-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/justinpersaud in version: 1.3.94-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
🚀 Deployed to staging by https://github.com/justinpersaud in version: 1.3.95-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.95-9 🚀
|
This was missed in commit ed9707a on Expensify#28422
Enables https on webpack dev server
Details
Enables https for local development
Before merging, we need to send out communication to #expensify-open-source and let them know as this will break their local devs until they setup the certificates.
After this PR is merged, merge this PR.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/320857
$
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android