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

Fix filepath generated by pika-treeshake on Windows #178

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

yukukotani
Copy link
Contributor

No description provided.

@yukukotani yukukotani force-pushed the feature/fix-win-filepath branch 13 times, most recently from a3ff4a6 to b9215b2 Compare January 28, 2020 08:09
@yukukotani yukukotani force-pushed the feature/fix-win-filepath branch from 1eff076 to 5765aef Compare February 2, 2020 16:18
@@ -60,6 +54,10 @@ for (const testName of readdirSync(__dirname)) {
// NOTE: We only compare files so that we give the test runner a more detailed diff.
return;
}
// NOTE: Skip source map on Windows, because generated source map has different newline code (CRLF).
Copy link
Contributor Author

@yukukotani yukukotani Feb 2, 2020

Choose a reason for hiding this comment

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

Skipping source map on Windows, because generated source map has different newline code (CRLF) from expected one on Windows.
I think the exact test of this is a responsibility of Rollup.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, stripWhitespace() was supposed to handle this. It was meant to strip all whitespace, including CRLF.

Can we try a different regex in stripWhitespace()? If that doesn't work, then adding this fallthrough is fine.

stdout.replace(/[ \t\r\n\f]+$/gm, '');

Copy link
Contributor Author

@yukukotani yukukotani Feb 5, 2020

Choose a reason for hiding this comment

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

@FredKSchott The problem was the escaped newline included in json value like '\\r\\n' or '\\n'.
Stripping it in 9024577, please review!

Copy link
Owner

Choose a reason for hiding this comment

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

Great! Thanks for putting in the extra time so that we get full coverage!

@yukukotani yukukotani marked this pull request as ready for review February 2, 2020 16:24
@yukukotani yukukotani changed the title [WIP] Fix filepath generated by pika-treeshake on Windows Fix filepath generated by pika-treeshake on Windows Feb 3, 2020
@FredKSchott
Copy link
Owner

LGTM! Added one comment just to check before we add that fall-through to ignore source map tests.

@yukukotani yukukotani force-pushed the feature/fix-win-filepath branch 6 times, most recently from d60e3ed to 6618f08 Compare February 5, 2020 12:52
@yukukotani yukukotani force-pushed the feature/fix-win-filepath branch from 6618f08 to 9024577 Compare February 5, 2020 12:59
@FredKSchott FredKSchott merged commit 2ce922d into master Feb 5, 2020
@FredKSchott FredKSchott deleted the feature/fix-win-filepath branch February 5, 2020 17:04
drwpow added a commit that referenced this pull request Jul 24, 2020
drwpow added a commit that referenced this pull request Jul 27, 2020
drwpow added a commit that referenced this pull request Jul 27, 2020
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