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

fix: Use the instance host instead of the current host #12226

Closed
wants to merge 0 commits into from

Conversation

MomentQYC
Copy link
Contributor

What

maybe relate #6724

Why

close #6724

Additional info (optional)

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Nov 2, 2023
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (66cecfa) 78.85% compared to head (d5de9a6) 78.95%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12226      +/-   ##
===========================================
+ Coverage    78.85%   78.95%   +0.10%     
===========================================
  Files          945      770     -175     
  Lines       101959    79039   -22920     
  Branches      8237     7803     -434     
===========================================
- Hits         80397    62409   -17988     
+ Misses       21562    16630    -4932     

see 175 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Nov 2, 2023

このPRによるapi.jsonの差分

差分はこちら

Get diff files from Workflow Page

@MomentQYC
Copy link
Contributor Author

There is currently an issue where the code implementation is accessing information directly from the API, which can lead to errors when running Test against the frontend like vitest (there is actually no problem, just no access to the API).

@MomentQYC MomentQYC marked this pull request as ready for review November 2, 2023 16:35
@syuilo
Copy link
Member

syuilo commented Nov 2, 2023

非同期的に決定される値をexportするのは良くなさそう

@MomentQYC
Copy link
Contributor Author

@syuilo Like this?

@tamaina
Copy link
Contributor

tamaina commented Nov 4, 2023

クライアントを開くたびにPOST to metaする実装には反対

@tamaina
Copy link
Contributor

tamaina commented Nov 4, 2023

テストもそのへん(xhr sendしてもサーバーが存在しない)でエラーが出ていそう

(だけどそもそも実装方針がよろしくない)

@MomentQYC
Copy link
Contributor Author

テストもそのへん(xhr sendしてもサーバーが存在しない)でエラーが出ていそう

As I said earlier, the API can't be connected (because it's not even running)

@MomentQYC
Copy link
Contributor Author

クライアントを開くたびにPOST to metaする実装には反対

So far I can only think of this one way to get the instance host

@tamaina
Copy link
Contributor

tamaina commented Nov 4, 2023

So far I can only think of this one way to get the instance host

私はそうは思わない

解決策の一つとしては、ビルド時に予めホストを指定すればよい。しかしこれは過去に廃止された、その理由は予めフロントエンドをビルドしてdockerイメージを提供したかったからだ。

(このプルリクエストの目的はapiのドメインとフロントエンドのドメインを分けたいということでいいのですよね?)

@tamaina
Copy link
Contributor

tamaina commented Nov 4, 2023

apiサーバーが応答しない場合はテスト以外にも多くあるはずなので、try-catchで適切にエラーを処理する必要がありそう

@tamaina
Copy link
Contributor

tamaina commented Nov 4, 2023

フロントエンドとバックエンドのドメインを分けると嬉しいことはわかっているけど、正式にサポートしましたと言うには様々な機能について動作を確認する必要がある(OAuthの動作等)

@MomentQYC
Copy link
Contributor Author

解決策の一つとしては、ビルド時に予めホストを指定すればよい。しかしこれは過去に廃止された、その理由は予めフロントエンドをビルドしてdockerイメージを提供したかったからだ。

What I want is to be able to switch the target domain name of the reverse proxy at any time, for example I can use it directly for a.com and abc.a.com without making any changes.

@MomentQYC
Copy link
Contributor Author

(このプルリクエストの目的はapiのドメインとフロントエンドのドメインを分けたいということでいいのですよね?)

Displays the host of the instance regardless of the current host

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept use domain different from web access
3 participants