Skip to content
This repository was archived by the owner on Apr 8, 2020. It is now read-only.

FIxing path separator to address #247 #248

Merged
merged 2 commits into from
Aug 8, 2016
Merged

FIxing path separator to address #247 #248

merged 2 commits into from
Aug 8, 2016

Conversation

aaronpowell
Copy link
Member

When investigating #247 I noticed that the NODE_PATH would look something like:

C:\tmp\.nvm\v6.3.1\node_modules:C:\_code\project\node_modules

The problem is that the path separator is : not ;.

This fix have resolve the issue in my testing.

Testing fix

  1. Create a new project using the yeoman generator
  2. Enable server side rendering
  3. Set NODE_PATH to have a value (eg: C:\tmp)
  4. Run the application and experience the error described in Can't find aspnet-prerendering when using a node version manager #247
  5. Apply patch
  6. Run the application successfully

@dnfclas
Copy link

dnfclas commented Aug 7, 2016

Hi @aaronpowell, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@aaronpowell
Copy link
Member Author

Hmm just noticed that : is what *nix seems to use as a path separator (testing using Bash on Ubuntu on Windows). My primary dev environment is Windows.

@aaronpowell
Copy link
Member Author

Fixing to use Path.PathSeparator like it should do to pick ; or :.

@SteveSandersonMS SteveSandersonMS merged commit 2a6465b into aspnet:dev Aug 8, 2016
@SteveSandersonMS
Copy link
Member

Fantastic! Thanks @aaronpowell for tracking this down and preparing a nice clean fix :) It's really great to have this one resolved.

@aaronpowell aaronpowell deleted the node-path-error branch August 8, 2016 03:16
@aaronpowell
Copy link
Member Author

@SteveSandersonMS what's the release model for this? Wanting to pull it into some samples 😉

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Aug 8, 2016

@aaronpowell I've pushed an updated set of beta packages (NodeServices, SpaServices, etc., all versioned 1.0.0-beta-000010), so your fix should be present in any new projects you create, or in existing ones if you force it to update the NuGet packages. Thanks again for the PR!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants