Skip to content

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

Open
daolou wants to merge 6 commits into
eggjs:masterfrom
daolou:master
Open

the host of the assets,if not set,it will be 127.0.0.1:${devServer.port} in development#33
daolou wants to merge 6 commits into
eggjs:masterfrom
daolou:master

Conversation

@daolou

@daolou daolou commented May 25, 2019

Copy link
Copy Markdown
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

codecov Bot commented May 25, 2019

Copy link
Copy Markdown

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

daolou commented May 25, 2019

Copy link
Copy Markdown
Author

@popomore
Hi, if you have time, review please

@popomore popomore left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

If use autoPort, how to specify the port in config

@daolou

daolou commented May 26, 2019

Copy link
Copy Markdown
Author

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

@hubcarl hubcarl left a comment

Copy link
Copy Markdown

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