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

Don't start server if target isn't browser #1204

Merged
merged 11 commits into from Apr 27, 2018
Merged

Conversation

DeMoorJasper
Copy link
Member

Closes #1088 Closes #1202 Closes #1005

@DeMoorJasper DeMoorJasper changed the title Don't server if target isn't browser Don't start server if target isn't browser Apr 17, 2018
src/Bundler.js Outdated
@@ -643,7 +643,9 @@ class Bundler extends EventEmitter {
}

async serve(port = 1234, https = false) {
this.server = await Server.serve(this, port, https);
if (this.options.target === 'browser') {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this check into the CLI specifically. If someone is using the Parcel API, and they call serve, we should always start a server even if the target is node. In the CLI, we can detect the target and just call bundle instead of serve there.

@codecov-io
Copy link

Codecov Report

Merging #1204 into master will decrease coverage by 0.9%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1204      +/-   ##
==========================================
- Coverage   88.28%   87.38%   -0.91%     
==========================================
  Files          77       77              
  Lines        4355     4113     -242     
==========================================
- Hits         3845     3594     -251     
- Misses        510      519       +9
Impacted Files Coverage Δ
src/transforms/htmlnano.js 72.22% <0%> (-27.78%) ⬇️
src/workerfarm/errorUtils.js 64.7% <0%> (-17.65%) ⬇️
src/transforms/posthtml.js 84.37% <0%> (-15.63%) ⬇️
src/visitors/dependencies.js 84.78% <0%> (-9.79%) ⬇️
src/transforms/babel.js 88.55% <0%> (-6.97%) ⬇️
src/visitors/globals.js 92.3% <0%> (-3.85%) ⬇️
src/assets/HTMLAsset.js 87.12% <0%> (-1.99%) ⬇️
src/utils/config.js 86.95% <0%> (-1.45%) ⬇️
src/visitors/fs.js 79.86% <0%> (-1.39%) ⬇️
src/utils/getTargetEngines.js 96.8% <0%> (-1.07%) ⬇️
... 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 bb7f4aa...c7d32fc. Read the comment docs.

src/cli.js Outdated

if (
(command.name() === 'serve' && command.target === undefined) ||
'browser'
Copy link
Member

Choose a reason for hiding this comment

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

not sure I understand this logic

Copy link
Member Author

Choose a reason for hiding this comment

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

that should've been

if (command.name() === 'serve' && 
  command.target === (undefined ||  'browser')) {
  // Start server
}

guess I made a typo, but fixed it now

@devongovett devongovett merged commit 9064b3b into master Apr 27, 2018
@devongovett devongovett deleted the fix-dontserver-on-node branch April 27, 2018 14:21
@felixrabe
Copy link

Not sure this is the right way to go. I use parcel serve --target electron for HMR. With this change (IIUC - didn't run it, just looking at the diff) that would not be possible anymore.

@devongovett
Copy link
Member

HMR still starts in watch mode. serve mode is just for running a web server as well.

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.

None yet

4 participants