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

Support Windows paths for pnp. #6447

Merged
merged 5 commits into from Oct 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -4,6 +4,10 @@ Please add one entry in this file for each change in Yarn's behavior. Use the sa

## Master

- Adds initial support for PnP on Windows

[#6447](https://github.com/yarnpkg/yarn/pull/6447) - [**John-David Dalton**](https://twitter.com/jdalton)

- Adds a special logic to PnP for ESLint compatibility (temporary, until [eslint/eslint#10125](https://github.com/eslint/eslint/issues/10125) is fixed)

[#6449](https://github.com/yarnpkg/yarn/pull/6449) - [**Maël Nison**](https://twitter.com/arcanis)
Expand Down
13 changes: 9 additions & 4 deletions src/config.js
Expand Up @@ -396,11 +396,16 @@ export default class Config {
}

if (process.platform === 'win32') {
if (this.plugnplayEnabled) {
this.reporter.warn(this.reporter.lang('plugnplayWindowsSupport'));
const cacheRootFolderDrive = path.parse(this._cacheRootFolder).root;
const lockfileFolderDrive = path.parse(this.lockfileFolder).root;

if (cacheRootFolderDrive !== lockfileFolderDrive) {
Copy link
Contributor

@vkrol vkrol Oct 8, 2018

Choose a reason for hiding this comment

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

There should be a case-insensitive check.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense! Can you open a PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like both cacheRootFolder and lockfileFolder can be user customized. Would it make more sense for user-provided values to be normalized at time of ingestion to avoid these kinds of one-off normalizations in other places in the code as well?

if (this.plugnplayEnabled) {
this.reporter.warn(this.reporter.lang('plugnplayWindowsSupport'));
}
Copy link
Contributor Author

@jdalton jdalton Oct 1, 2018

Choose a reason for hiding this comment

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

Now may be a good time to think on other platform specific path things that might make relative paths less portable from platform to platform. Off the top of my head there is also long paths (namespaced paths) in Windows.

Update:

I think the root check above may already handle namespaced paths.

Copy link
Member

Choose a reason for hiding this comment

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

I guess if the need arise we could just have a plugnplay-use-absolute settings that would use absolute paths for everything outside of __dirname. Since getPackageInformation already returns absolute paths no matter what, it should be backward compatible.

this.plugnplayEnabled = false;
this.plugnplayPersist = false;
}
this.plugnplayEnabled = false;
this.plugnplayPersist = false;
}

this.plugnplayShebang = String(this.getOption('plugnplay-shebang') || '') || '/usr/bin/env node';
Expand Down
3 changes: 1 addition & 2 deletions src/reporters/lang/en.js
Expand Up @@ -357,8 +357,7 @@ const messages = {

unplugDisabled: "Packages can only be unplugged when Plug'n'Play is enabled.",

plugnplayWindowsSupport:
"Plug'n'Play is ignored on Windows for now - contributions welcome! https://github.com/yarnpkg/yarn/issues/6402",
plugnplayWindowsSupport: "Plug'n'Play on Windows doesn't support the cache and project to be kept on separate drives",

packageInstalledWithBinaries: 'Installed $0 with binaries:',
packageHasBinaries: '$0 has binaries:',
Expand Down
20 changes: 16 additions & 4 deletions src/util/generate-pnp-map-api.tpl.js
Expand Up @@ -22,15 +22,18 @@ const blacklistedLocator = {name: NaN, reference: NaN};
const patchedModules = new Map();
const fallbackLocators = [topLevelLocator];

// Splits a require request into its components, or return null if the request is a file path
const pathRegExp = /^(?!\.{0,2}(?:\/|$))((?:@[^\/]+\/)?[^\/]+)\/?(.*|)$/;
// Matches backslashes of Windows paths
const backwardSlashRegExp = /\\/g;

// Matches if the path must point to a directory (ie ends with /)
const isDirRegExp = /\/$/;

// Matches if the path starts with a valid path qualifier (./, ../, /)
// eslint-disable-next-line no-unused-vars
const isStrictRegExp = /^\.{0,2}\//;

// Matches if the path must point to a directory (ie ends with /)
const isDirRegExp = /\/$/;
// Splits a require request into its components, or return null if the request is a file path
const pathRegExp = /^(?!\.{0,2}(?:\/|$))((?:@[^\/]+\/)?[^\/]+)\/?(.*|)$/;

// Keep a reference around ("module" is a common name in this context, so better rename it to something more significant)
const pnpModule = module;
Expand Down Expand Up @@ -229,6 +232,15 @@ function makeFakeModule(path) {
return fakeModule;
}

/**
* Normalize path to posix format.
*/

// eslint-disable-next-line no-unused-vars
function normalizePath(fsPath) {
return process.platform === 'win32' ? fsPath.replace(backwardSlashRegExp, '/') : fsPath;
}

/**
* Forward the resolution to the next resolver (usually the native one)
*/
Expand Down
12 changes: 9 additions & 3 deletions src/util/generate-pnp-map.js
Expand Up @@ -11,6 +11,8 @@ const crypto = require('crypto');
const invariant = require('invariant');
const path = require('path');

const backwardSlashRegExp = /\\/g;

const OFFLINE_CACHE_EXTENSION = `.zip`;

type PackageInformation = {|
Expand Down Expand Up @@ -100,7 +102,7 @@ function generateFindPackageLocator(packageInformationStores: PackageInformation

// Generate a function that, given a file path, returns the associated package name
code += `exports.findPackageLocator = function findPackageLocator(location) {\n`;
code += ` let relativeLocation = path.relative(__dirname, location);\n`;
code += ` let relativeLocation = normalizePath(path.relative(__dirname, location));\n`;
code += `\n`;
code += ` if (!relativeLocation.match(isStrictRegExp))\n`;
code += ` relativeLocation = \`./\${relativeLocation}\`;\n`;
Expand Down Expand Up @@ -136,7 +138,7 @@ async function getPackageInformationStores(
const blacklistedLocations: Set<string> = new Set();

const getCachePath = (fsPath: string) => {
const cacheRelativePath = path.relative(config.cacheFolder, fsPath);
const cacheRelativePath = normalizePath(path.relative(config.cacheFolder, fsPath));

// if fsPath is not inside cacheRelativePath, we just skip it
if (cacheRelativePath.match(/^\.\.\//)) {
Expand Down Expand Up @@ -164,8 +166,12 @@ async function getPackageInformationStores(
return path.resolve(offlineCacheFolder, `${cacheEntry}${OFFLINE_CACHE_EXTENSION}`, internalPath.join('/'));
};

const normalizePath = (fsPath: string) => {
return process.platform === 'win32' ? fsPath.replace(backwardSlashRegExp, '/') : fsPath;
};

const normalizeDirectoryPath = (fsPath: string) => {
let relativePath = path.relative(targetDirectory, resolveOfflineCacheFolder(fsPath));
let relativePath = normalizePath(path.relative(targetDirectory, resolveOfflineCacheFolder(fsPath)));

if (!relativePath.match(/^\.{0,2}\//)) {
relativePath = `./${relativePath}`;
Expand Down