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

Watch not working on windows #65

Closed
spectras opened this issue Mar 20, 2020 · 9 comments
Closed

Watch not working on windows #65

spectras opened this issue Mar 20, 2020 · 9 comments
Labels

Comments

@spectras
Copy link

spectras commented Mar 20, 2020

Using version 7.0.3.
Past the initial preprocessing, no file change ever trigger test again.

I traced the issue to the matching in the watcher. It passes down changed file path to Karma's fileList. In turn, the fileList does this:

//karma\lib\file-list.js:48
_findFile (path, pattern) {
  if (!path || !pattern) return
  return this._getFilesByPattern(pattern.pattern).find((file) => file.originalPath === path)
}

The issue is:

  • path is a windows path, in my case, D:\home\projects\skeleton\skeleton-lib\tests\index.js.
  • file.originalPath is not: 'D:/home/projects/skeleton/skeleton-lib/tests/index.js'.

It looks like Karma's fileList expects the given path to always be posix format, and karma-rollup-preprocessor sends windows format instead.

@spectras
Copy link
Author

spectras commented Mar 20, 2020

Note: a quick-and-dirty change in karma-rollup-preprocessor/lib/Watcher.js like this:

-		this.emitter._fileList.changeFile(path, true)
+		this.emitter._fileList.changeFile(path.replace(/\\/g, '/'), true)

…makes it work on my system.

That would however break projects that actually use backslashes in file names on posix systems. Perhaps check the value of require('path').sep instead of replacing unconditionally?

@jlmakes jlmakes added the bug label Mar 21, 2020
@jlmakes
Copy link
Owner

jlmakes commented Mar 23, 2020

Something like this?

const sep = require('path').sep

//...

this.emitter._fileList.changeFile(sep !== '/' ? path.replace(/\\/g, '/') : path, true)

@Lalem001
Copy link
Contributor

@jlmakes path.normalize might work here. Not at my computer, so cannot double check.

@Lalem001
Copy link
Contributor

Lalem001 commented Mar 23, 2020

Took a look, and my above comment with path.normalize will not work.

Take a look instead at slash.

Convert Windows backslash paths to slash paths: foo\\barfoo/bar

@jlmakes
Copy link
Owner

jlmakes commented Mar 23, 2020

It's basically a wrapper around the same string replace:

// slash.js

'use strict';
module.exports = path => {
	const isExtendedLengthPath = /^\\\\\?\\/.test(path);
	const hasNonAscii = /[^\u0000-\u0080]+/.test(path);

	if (isExtendedLengthPath || hasNonAscii) {
		return path;
	}

	return path.replace(/\\/g, '/');
};

I understand testing for ASCII, but I had to search for "extended length paths":
https://stackoverflow.com/questions/21194530/what-does-mean-when-prepended-to-a-file-path

@Lalem001
Copy link
Contributor

I know, was just thinking that it could potentially handle the edge cases for you if they ever came up.

@jlmakes
Copy link
Owner

jlmakes commented Mar 23, 2020

I'm generally cautious of adding dependencies, but I'm not opposed to it. I just don't fully understand if those cases are overly defensive for Karma and Rollup. As in, will non-ASCII or extended length paths occur in a JavaScript code base, or the build process therein?

That would however break projects that actually use backslashes in file names on posix systems. — @spectras

This would still need to be addressed in the implementation. So, something like this?

const slash = require('slash')
const sep = require('path').sep

//...

this.emitter._fileList.changeFile(sep !== '/' ? slash(path) : path, true)

@Lalem001
Copy link
Contributor

Just want to point out that \u0000 does show up when certain rollup plugins are involved.

@spectras
Copy link
Author

spectras commented Mar 24, 2020

Good news and bad news!

Bad news: 5668d68 does not work here.

Good news: the fix in it works.

  • After seeing 5668d68 did not work, I tried 7.0.4, and I have the same problem in it.
  • So I downgraded to 7.0.3 and backported the commit. This worked and I confirm it fixes the issue.

So, the fix works, but 7.0.4 introduced another issue. I'll open a separate topic.

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

No branches or pull requests

3 participants