Skip to content

feat: support multiple dev servers by using random temp directories #354

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

Closed
wants to merge 4 commits into from

Conversation

meteorlxy
Copy link
Member

@meteorlxy meteorlxy commented May 7, 2018

Current global vuepress does not support multiple instances.

In other words, we cannot open multiple dev server using global installed vuepress.

The reason is we are now using a fixed/single temp directory, i.e. lib/app/.temp. Each project will overwrite the same temp directory, which will cause collision.

For enhancement, I introduce tmp module, which is widely used by many projects.

@meteorlxy
Copy link
Member Author

#355

Copy link
Member

@ulivz ulivz left a comment

Choose a reason for hiding this comment

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

Since VuePress is still in the early iterations, I still think that the temp dirs should be in placed in local package's directory.

To support multiple temp dirs, you can simply suffixed the temp dir with a hash or timestamp for each instance.

yarn.lock Outdated
@@ -6222,7 +6222,7 @@ tiny-emitter@^2.0.0:

tmp@^0.0.33:
version "0.0.33"
resolved "https://registry.yarnpkg.com/tmp/-/tmp-0.0.33.tgz#6d34335889768d21b2bcda0aa277ced3b1bfadf9"
resolved "http://registry.npm.taobao.org/tmp/download/tmp-0.0.33.tgz#6d34335889768d21b2bcda0aa277ced3b1bfadf9"
Copy link
Member

Choose a reason for hiding this comment

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

This change shouldn't be included here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, forget to add -g when change the registry

@meteorlxy
Copy link
Member Author

meteorlxy commented May 7, 2018

@ulivz

Changed to local package's dir.

@ycmjason
Copy link
Contributor

ycmjason commented May 7, 2018

I wonder why would we need to spawn multiple dev servers?

@ulivz
Copy link
Member

ulivz commented May 7, 2018

Same question, why would we need to spawn multiple dev servers?

@meteorlxy
Copy link
Member Author

Good question, why hexo & jekyll support multiple dev servers?

@ulivz
Copy link
Member

ulivz commented May 7, 2018

Hmmm, do you mean that we must support others' support? Where is the actual meaning?

@meteorlxy
Copy link
Member Author

meteorlxy commented May 7, 2018

LOL, don't mind my word.

Users may have different requirements. For me, I'm deving the official repo and my own custom theme in the mean time, so I have to restart and restart.

Or I have another question: why we restrict users to open only one dev server?

@hmatalonga
Copy link

@meteorlxy

While it is not strictly necessary... I agree it can be handy to have that possibility 😃

@ulivz
Copy link
Member

ulivz commented May 7, 2018

Shouldn't it be a feature request first? If you just submitted a PR and hoped to be merged as soon as possible, then we will discuss whether we need to do it. Will you put the cart before the horse?

BTW, I don't want and DON'T have time to argue with you at all. If you want to support a feature, please submit a feature request first.

@meteorlxy
Copy link
Member Author

I did, sir #355... And you tagged it as enhancement 😓

@ulivz
Copy link
Member

ulivz commented May 7, 2018

@meteorlxy

We just want to know why we need to support this feature. in my opinion, this makes core a little more complex and create a unpleasant directory, and 99% users will only need one instance. so if you can give us a convincing reason, I will support you. But as a reviewer, you just kept asking me.

I tagged a enhancementto #355 is that the description doesn't look like a feature request but also not bug.

I think we should follow such a process: Why need it => How to do it.

@ulivz
Copy link
Member

ulivz commented May 7, 2018

And, can you confirm that will the old temp file be deleted when reboot?

@meteorlxy
Copy link
Member Author

meteorlxy commented May 7, 2018

@ulivz They will be deleted immediately after the process exits.

However, there may be some exceptions that make the process interrupt unexpectedly, or the machine shutdown accidently, then the temp files will remain there.

If we use system tmp directory as my first commit, we don't have to worry about it.

If we use local package directory as you suggested, it could be a potential problem that we should delete them manually.

@ulivz
Copy link
Member

ulivz commented May 7, 2018

We can simply implement it with native node.js. since we didn't need tmp's other features.

And delete the .temp-* dirs before prepare.

@meteorlxy
Copy link
Member Author

meteorlxy commented May 7, 2018

After discussion on slack, so far we (@ulivz , @ycmjason and I) have some conclusion:

  • Multiple dev servers is necessary in these possible scenarios:
    • Developing multiple vuepress projects in the mean time, or trying to campare some changes with each other.
    • Multiple users access to one remote machine, and start vuepress dev server in the mean time.
  • Although tmp module can cleanup the tempfile after the process exits, if the vuepress process was killed by accident, the temp files will still remain there. So we would prefer to store the temp files to OS's temp dir. (But we cannot determine if this is a good solution. cc @yyx990803 )

@meteorlxy meteorlxy changed the title feat: support multiple instance feat: support multiple dev servers by using random temp directories May 15, 2018
@ulivz ulivz force-pushed the master branch 3 times, most recently from 14d9013 to c7c0cd9 Compare June 8, 2018 19:11
@ulivz ulivz force-pushed the master branch 5 times, most recently from c992e38 to c2eaff3 Compare July 12, 2018 18:01
@ulivz ulivz force-pushed the master branch 4 times, most recently from 0c3bc3a to cf1e6fc Compare July 28, 2018 08:12
@meteorlxy meteorlxy mentioned this pull request Aug 11, 2018
@meteorlxy meteorlxy closed this Sep 26, 2018
@meteorlxy meteorlxy deleted the use-temp branch March 13, 2019 08:46
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.

4 participants