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

Add server reference to bundler #822

Merged
merged 2 commits into from
Feb 20, 2018

Conversation

fusepilot
Copy link
Contributor

I need to get the development server's port number for use in a plugin. Attaching the server to the bundler allows me to call bundler.server.address().port. This seems to be the only way to access it since the server's port can vary if the user provided port is already being used.

brandon93s
brandon93s previously approved these changes Feb 16, 2018
@brandon93s
Copy link
Contributor

Actually, .serve() returns the server. Is that not sufficient?

@brandon93s brandon93s dismissed their stale review February 16, 2018 03:33

Afterthought.

@codecov-io
Copy link

Codecov Report

Merging #822 into master will decrease coverage by 3.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #822      +/-   ##
==========================================
- Coverage   93.41%   90.16%   -3.26%     
==========================================
  Files          68       68              
  Lines        3753     3181     -572     
==========================================
- Hits         3506     2868     -638     
- Misses        247      313      +66
Impacted Files Coverage Δ
src/Bundler.js 92.83% <100%> (+0.02%) ⬆️
src/Asset.js 66.94% <0%> (-31.36%) ⬇️
src/utils/installPackage.js 77.41% <0%> (-19.46%) ⬇️
src/utils/config.js 76.92% <0%> (-17.45%) ⬇️
src/assets/StylusAsset.js 72.5% <0%> (-16.25%) ⬇️
src/Logger.js 81.9% <0%> (-12.39%) ⬇️
src/assets/GlobAsset.js 90% <0%> (-8.75%) ⬇️
src/visitors/matches-pattern.js 92% <0%> (-8%) ⬇️
src/utils/localRequire.js 95.23% <0%> (-4.77%) ⬇️
src/assets/RawAsset.js 90% <0%> (-4.12%) ⬇️
... and 18 more

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 852f6b3...842e955. Read the comment docs.

@fusepilot
Copy link
Contributor Author

Wouldn't .serve() spin up a new server instance? I tried it and the build errors with

loadedAssets is not iterable
  at Bundler.bundle (.../parcel-bundler/src/Bundler.js:179:61)

@brandon93s
Copy link
Contributor

for use in a plugin

Missed this part. Expecting the CLI to continue to call serve and having the server be available to your plugin by keeping a reference to it. 👍

src/Bundler.js Outdated
@@ -575,6 +575,7 @@ class Bundler extends EventEmitter {

async serve(port = 1234, https = false) {
let server = await Server.serve(this, port, https);
this.server = server;
this.bundle();
return server;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do:

     this.server = await Server.serve(this, port, https);
     this.bundle();
     return this.server;

@fusepilot
Copy link
Contributor Author

I don't see how this change could cause the AppVeyor build to fail. Do I need to do anything?

@DeMoorJasper
Copy link
Member

@fusepilot it's a rust test that fails from time to time, no need to worry about it

@devongovett devongovett merged commit a242471 into parcel-bundler:master Feb 20, 2018
@fusepilot fusepilot deleted the bundler-server-reference branch February 20, 2018 07:09
devongovett pushed a commit that referenced this pull request Oct 15, 2018
* add server reference to bundler

* remove let
devongovett pushed a commit that referenced this pull request Oct 15, 2018
* add server reference to bundler

* remove let
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.

6 participants