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

Only Use Contextify When Required #9

Merged
merged 2 commits into from
Jan 27, 2016
Merged

Conversation

jmurphyau
Copy link
Contributor

I didn't realise my other PR was closed.. It seems they close themselves when the branch is exactly the same as the branch you're doing the PR against..

This PR adds the host option and uses/installs contextify only when it's needed (< node 0.10.x)

@tomdale
Copy link
Contributor

tomdale commented Jan 26, 2016

Thanks @jmurphyau, can you move the host change into a separate PR? There are still a few commented out lines that should be cleaned up as well. Lastly, please use the style isLegacyVM instead of isLegacyVM. I will try to add a style guide so that isn't ambiguous. Thanks!

@jmurphyau
Copy link
Contributor Author

@tomdale should look a bit better now

@jmurphyau
Copy link
Contributor Author

Also, just an FYI - that plugin you mentioned about installing dependancies based on node version.. What it did was install a version of npm (which could be (and in my case was) a different version to the users current npm) as a dependancy and then programatically install the dependancies.. The tried this and I had two issues with it:

  1. It downloaded NPM as a dependancy.. You mentioned the length of time for building the contextify plugin - it almost seemed like downloading NPM took the same duration or longer.
  2. NPM has been recently updated to show a progress bar sort of things rather than the previous style (I think it output a list of things that were happening when they were happening?) - the plugin used an NPM that produced the older style output.. It threw me off for a split second before realising what it was - especially since I was switching between node and NPM versions to test it out..

So instead of using that I've gone with a script that checks the isLegacyVM return result and produces an exit code based on that together with post install config that will install contextify if required

@tomdale
Copy link
Contributor

tomdale commented Jan 27, 2016

Awesome! Thank you for all the hard work. I'm very excited about this change.

tomdale added a commit that referenced this pull request Jan 27, 2016
Only Use Contextify When Required
@tomdale tomdale merged commit 5b89456 into ember-fastboot:master Jan 27, 2016
@tomdale
Copy link
Contributor

tomdale commented Feb 1, 2016

@jmurphyau I had to revert this because it causes npm installs of this package to fail if it's not a legacy VM.

@jmurphyau
Copy link
Contributor Author

really? hmm thats odd.. I use the latest node/npm - which fall into the category you mention - and it appeared to work fine for me.. Do you have any example of the error that appears

bantic pushed a commit to bantic/ember-fastboot-server that referenced this pull request Apr 28, 2016
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.

2 participants