remove empty worksheet[0] from _worksheets

[pookong]

_worksheets are saved as a loose array with undefined item[0]

when counting number of worksheets in xform models the .length is used - includes item[0]
when inserting worksheets into xml then .forEach method is used - excludes item[0]

this results in docProps/app.xml with data for a workbook with a single worksheet foo like:

<TitlesOfParts>
  <vt:vector size="2" baseType="lpstr">
    <vt:lpstr>foo</vt:lpstr>
  </vt:vector>
</TitlesOfParts>

error being in vt:vector of size 2 with only one item inside

[BridgeAR]

I just ran into a similar issue:

TypeError: Cannot read property 'name' of undefined
  at /home/ruben/repos/finturanode/node_modules/exceljs/lib/stream/xlsx/workbook-writer.js:197:25

The reason is that the function nextId begins counting the sheets by one. But arrays start with zero and otherwise the sheets are saved in a sparsed array with the first entry never existing (this is also leading to bad performance due to v8 not being able to optimize this).

So a proper fix would not be to filter everything but to just fix that function. Just set i to zero and remove the || 1 in the return value.

A simple regression test would be something like:

const Stream = require('stream')
const Excel = require('exceljs')
const stream = new Stream.Writable({ write: function noop () {} })
const workbook = new Excel.stream.xlsx.WorkbookWriter({
  stream: stream
})
workbook.addWorksheet('first')
workbook.getWorksheet('w00t') // cannot read property 'name' of undefined

[pookong]

@BridgeAR thanks for suggestion!

while it seems reasonable to index from 0, i fear such a change of interface would break a lot of dependent code since it is possible to access sheets using their generated id:

workbook.addWorksheet('first')
workbook.getWorksheet(0) //results in undefined
workbook.getWorksheet(1) //results in 'first' worksheet

there seem to be however more places where the loose array poses a problem - i will look into it deeper next week

ps: you are 100% right filtering everything is an overkill - simply slicing the first item would be enough

[BridgeAR]

@pookong yes, this is a minimal behavior change but at least right now I do not see a better way to do this.

It would be nice if semver would be used but this is not the case, so breaking changes might be in any update but I might be wrong.

And this is a very trivial bug fix that fixes lots of side effects opposed to quite a few difficulties to work around this and keep the current behavior.

@guyonroche could you give us a brief update on your view to this issue? :-)

[guyonroche]

@pookong, @BridgeAR
Welcome to the wonders that is xlsx file format :-)
Many of the indexes in the document are indeed 1 based - which is why nextId starts at one and why workbook.getWorksheet(0) will always be undefined.

I've added unit-tests to cover the cannot read property 'name' of undefined issue

I'll investigate the effects of this PR this weekend

[guyonroche]

I think filtering is fine - after all, the arrays of worksheets will typically be small.
I would prefer not to change the behaviour of workbook.getWorksheet(0) as that would mean a breaking change. From what I can see, this PR doesn't make any interface changes and it does fix the vector length issue.

[BridgeAR]

@guyonroche well in that case I recommend to change the getWorksheet method.

There does not have to be a breaking change by just fixing the getter in the calling method. But using sparse arrays is not a good idea and will likely have more side effects than here visible. And I guess you do not want to special handle zero everytime.

So I'd say a better solution would be something like:

class Workbook {
  // ...
  getWorksheet (index) {
    if (typeof index === 'number') {
      if (index <= 0) {
        throw new TypeError('The index has to be 1 or above')
      }
      index--
      // Do original stuff
    } else ...
  }
}