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

the host of the assets,if not set,it will be 127.0.0.1:${devServer.port} in development #33

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

daolou
Copy link

@daolou daolou commented May 25, 2019

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

C89CF34F-933D-4C04-97E8-4E8EC13C0C01

Case:

  1. SSO( Single Sign-On )while use same domian cookies, in development:
    • map host, add domain 127.0.0.1 to /etc/hosts
    • assets server url should be domain
  2. Others

So, if it setted the url, it will be it, if it not set the url, then use default 127.0.0.1

@codecov
Copy link

codecov bot commented May 25, 2019

Codecov Report

Merging #33 into master will decrease coverage by 0.50%.
The diff coverage is 80.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #33      +/-   ##
===========================================
- Coverage   100.00%   99.49%   -0.51%     
===========================================
  Files           10       10              
  Lines          196      198       +2     
===========================================
+ Hits           196      197       +1     
- Misses           0        1       +1     
Impacted Files Coverage Δ
config/config.default.js 100.00% <ø> (ø)
app.js 95.23% <80.00%> (-4.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update feb5f41...ce7bf24. Read the comment docs.

@daolou
Copy link
Author

daolou commented May 25, 2019

@popomore
Hi, if you have time, review please

Copy link
Member

@popomore popomore left a comment

Choose a reason for hiding this comment

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

Add a testcase and delete the package-lock

@popomore
Copy link
Member

If use autoPort, how to specify the port in config

@daolou
Copy link
Author

daolou commented May 26, 2019

If use autoPort, how to specify the port in config

this:

    if (!assetsConfig.url) {
      assetsConfig.url = 'http://127.0.0.1:' + port;
    } else {
      assetsConfig.url = assetsConfig.url + ':' + port;
    }

In addition, if someone want use himself url, in that case, he/she should know the port.

@daolou daolou requested a review from popomore August 7, 2020 03:13
@atian25 atian25 requested a review from hubcarl August 7, 2020 03:25
Copy link

@hubcarl hubcarl left a comment

Choose a reason for hiding this comment

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

fix pr merge conflict

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