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

Some filenames causes EXCEL to report error #2722

Open
lemonbob opened this issue Jun 19, 2022 · 3 comments
Open

Some filenames causes EXCEL to report error #2722

lemonbob opened this issue Jun 19, 2022 · 3 comments

Comments

@lemonbob
Copy link

Hi, attached is the file that has thrown an error by EXCEL. The fileName is: RiskThemes_ Bubble Chart.xlsx.

I tried exporting with a fileName of test.xlsx and it is fine, so this must be a filename issue. I don't see anything wrong with the filename RiskThemes_ Bubble Chart.xlsx - it seems perfectly valid to me? Any ideas?

Attached is the file.
RiskThemes_ Bubble Chart.xlsx

@SheetJSDev
Copy link
Contributor

: is missing from the bad characters list, feel free to submit a PR

--- a/bits/71_wbcommon.js
+++ b/bits/71_wbcommon.js
@@ -113,7 +113,7 @@ function safe1904(wb/*:Workbook*/)/*:string*/ {
        return parsexmlbool(wb.Workbook.WBProps.date1904) ? "true" : "false";
 }
 
-var badchars = /*#__PURE__*/"][*?\/\\".split("");
+var badchars = /*#__PURE__*/":][*?\/\\".split("");
 function check_ws_name(n/*:string*/, safe/*:?boolean*/)/*:boolean*/ {
        if(n.length > 31) { if(safe) return false; throw new Error("Sheet names cannot exceed 31 chars"); }
        var _good = true;

@lemonbob
Copy link
Author

lemonbob commented Jun 20, 2022

Hi,

I had a look at the xlsx.js file and I wondered what the purpose of check_ws_name safe param was, please? It is never used? Also the return value _good never seems to be used?

I also wondered, rather than throwing an Error when the filename contains "badchars" it would, perhaps, be better UX to simply replace them for the user?

This would have to be done in book_append_worksheet and the various write methods so is a bigger change - e.g.

name = name.replace(/[\\\/\?\*\[\]\:]/g, '_');

To maintain the logic exactly as is with the throw, the following appears to be all we need as the safe parameter is always undefined and the return value is never used

function check_ws_name(n) {
	let matches = n.match(/[\\\/\?\*\[\]\:]/g);
	if (matches != undefined) throw new Error("Sheet name cannot contain : \\ / ? * [ ] :");	
}

In any event, forEach and looping through badchars converted to an array seems a little clunky. Please advise which of the above solutions would be preferable?

@SheetJSDev
Copy link
Contributor

The latter solution is preferable (modifying check_ws_name).

It's preferable to throw the error message as that is consistent with Excel. If you try to rename a worksheet to contain a : character, Excel throws an error The name does not contain any of the following characters: : \ / ? * [ or ]

The main reason not to autocorrect the worksheet name involves features like defined names and formulae. Suppose you were unaware of the Excel limitation and wrote a formula in a cell that used a cross-sheet reference (='abc:def'!A1). This formula would be incorrect if the worksheet is renamed. It is possible to write the formula in XLSX, but Excel treats it like an external reference. Small example: https://jsfiddle.net/dtshugz7/

var ws = XLSX.utils.aoa_to_sheet([[{t:"n",f:"'abc:def'!A1"}]]);
var wb = XLSX.utils.book_new();
XLSX.utils.book_append_sheet(wb, ws, "Sheet1");
XLSX.writeFile(wb, "issue2722.xlsx");

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

No branches or pull requests

2 participants