Skip to content

Properly assigning styles by deep copy #722

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.

Sign up for GitHub

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

Merged
alubbe merged 2 commits into from
Apr 3, 2019
Merged

Properly assigning styles by deep copy #722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d7f65a
property assigning styles by deep copy
Jan 14, 2019
b159ab0
reduce async
Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 30 additions & 30 deletions lib/doc/row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@

'use strict';

var _ = require('../utils/under-dash');
const _ = require('../utils/under-dash');

var Enums = require('./enums');
var colCache = require('./../utils/col-cache');
var Cell = require('./cell');
const Enums = require('./enums');
const colCache = require('./../utils/col-cache');
const Cell = require('./cell');

var Row = module.exports = function(worksheet, number) {
const Row = module.exports = function (worksheet, number) {
this._worksheet = worksheet;
this._number = number;
this._cells = [];
Expand Down Expand Up @@ -49,9 +49,9 @@ Row.prototype = {

// given {address, row, col}, find or create new cell
getCellEx: function(address) {
var cell = this._cells[address.col - 1];
let cell = this._cells[address.col - 1];
if (!cell) {
var column = this._worksheet.getColumn(address.col);
const column = this._worksheet.getColumn(address.col);
cell = new Cell(this, column, address.address);
this._cells[address.col - 1] = cell;
}
Expand All @@ -62,7 +62,7 @@ Row.prototype = {
getCell: function(col) {
if (typeof col === 'string') {
// is it a key?
var column = this._worksheet.getColumnKey(col);
const column = this._worksheet.getColumnKey(col);
if (column) {
col = column.number;
} else {
Expand All @@ -79,11 +79,11 @@ Row.prototype = {

// remove cell(s) and shift all higher cells down by count
splice: function(start, count) {
var inserts = Array.prototype.slice.call(arguments, 2);
var nKeep = start + count;
var nExpand = inserts.length - count;
var nEnd = this._cells.length;
var i, cSrc, cDst;
const inserts = Array.prototype.slice.call(arguments, 2);
const nKeep = start + count;
const nExpand = inserts.length - count;
const nEnd = this._cells.length;
let i, cSrc, cDst;

if (nExpand < 0) {
// remove cells
Expand Down Expand Up @@ -121,8 +121,8 @@ Row.prototype = {
options = null;
}
if (options && options.includeEmpty) {
var n = this._cells.length;
for (var i = 1; i <= n; i++) {
const n = this._cells.length;
for (let i = 1; i <= n; i++) {
iteratee(this.getCell(i), i);
}
} else {
Expand Down Expand Up @@ -152,7 +152,7 @@ Row.prototype = {

// return a sparse array of cell values
get values() {
var values = [];
const values = [];
this._cells.forEach(function(cell) {
if (cell && (cell.type !== Enums.ValueType.Null)) {
values[cell.col] = cell.value;
Expand All @@ -168,7 +168,7 @@ Row.prototype = {
if (!value) {
// empty row
} else if (value instanceof Array) {
var offset = 0;
let offset = 0;
if (value.hasOwnProperty('0')) {
// contiguous array - start at column 1
offset = 1;
Expand Down Expand Up @@ -207,7 +207,7 @@ Row.prototype = {
return this._cells.length;
},
get actualCellCount() {
var count = 0;
let count = 0;
this.eachCell(function() {
count++;
});
Expand All @@ -216,8 +216,8 @@ Row.prototype = {

// get the min and max column number for the non-null cells in this row or null
get dimensions() {
var min = 0;
var max = 0;
let min = 0;
let max = 0;
this._cells.forEach(function(cell) {
if (cell && (cell.type !== Enums.ValueType.Null)) {
if (!min || (min > cell.col)) {
Expand Down Expand Up @@ -294,12 +294,12 @@ Row.prototype = {

// =========================================================================
get model() {
var cells = [];
var min = 0;
var max = 0;
const cells = [];
let min = 0;
let max = 0;
this._cells.forEach(function(cell) {
if (cell) {
var cellModel = cell.model;
const cellModel = cell.model;
if (cellModel) {
if (!min || (min > cell.col)) {
min = cell.col;
Expand Down Expand Up @@ -329,21 +329,21 @@ Row.prototype = {
throw new Error('Invalid row number in model');
}
this._cells = [];
var previousAddress;
let previousAddress;
value.cells.forEach(cellModel => {
switch (cellModel.type) {
case Cell.Types.Merge:
// special case - don't add this types
break;
default:
var address;
let address;
if (cellModel.address) {
address = colCache.decodeAddress(cellModel.address);
} else if (previousAddress) {
// This is a <c> element without an r attribute
// Assume that it's the cell for the next column
var row = previousAddress.row;
var col = previousAddress.col + 1;
const row = previousAddress.row;
const col = previousAddress.col + 1;
address = {
row: row,
col: col,
Expand All @@ -352,7 +352,7 @@ Row.prototype = {
};
}
previousAddress = address;
var cell = this.getCellEx(address);
const cell = this.getCellEx(address);
cell.model = cellModel;
break;
}
Expand All @@ -367,6 +367,6 @@ Row.prototype = {
this.hidden = value.hidden;
this.outlineLevel = value.outlineLevel || 0;

this.style = value.style || {};
this.style = value.style && JSON.parse(JSON.stringify(value.style)) || {};
Copy link
Member Author

@Siemienik Siemienik Jan 17, 2019

Choose a reason for hiding this comment

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

fix is here ;)

Copy link
Member

@alubbe alubbe Jan 17, 2019

Choose a reason for hiding this comment

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

I'm not a big fan of this, since there's a couple of subtle ways this can fail or lose things depending on the shape of the object. do we know what values each key of value.style can hold? is it just a flat object of "some string": "another string" or can it also hold numbers, arrays, functions or other objects?

Copy link
Member Author

@Siemienik Siemienik Jan 18, 2019

Choose a reason for hiding this comment

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

it is only object that keep configuration of styles
there aren't any functions or getters/setters

Copy link
Member

@alubbe alubbe Jan 21, 2019

Choose a reason for hiding this comment

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

gotcha. Can you help me understand the performance impact here?
JSON.parse(JSON.stringify()) is quite slow and it would suck if this actually runs for all (or most) rows. Also, it increases the memory overhead a bit. How many rows will have this property?

Copy link
Member Author

@Siemienik Siemienik Feb 18, 2019

Choose a reason for hiding this comment

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

personally I think it's much better to write style cloning function in utils ;)

Copy link

@BridgeAR BridgeAR Mar 12, 2019

Choose a reason for hiding this comment

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

Using JSON is indeed pretty slow to copy things. You could use e.g., https://github.com/davidmarkclements/rfdc for now (I would normally just recommend to use object spread but that's not available in versions below 8 and slow before v.10 or v.11 😄).

@alubbe the current API does not seem to have performance in mind. If the user changes any input that should be registered in a clean way so all internals can work side effect free. This does not seem to be possible without the penalty here. Using getters and setters might reduce the overhead by only copying values that have to be changed but as far as I see it that would probably be pretty brittle with the current code.

Copy link
Member

@alubbe alubbe Apr 3, 2019

Choose a reason for hiding this comment

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

I agree - let's merge it and then actually benchmark why this library is slow and fix those, as opposed to pre-optimizing a bugfix

}
};
Binary file added spec/integration/data/test-row-styles.xlsx
View file
Open in desktop
Binary file not shown.
55 changes: 39 additions & 16 deletions spec/integration/workbook-xlsx-reader.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
'use strict';

var verquire = require('../utils/verquire');
var testutils = require('../utils/index');
var fs = require('fs');
var expect = require('chai').expect;
const verquire = require('../utils/verquire');
const testutils = require('../utils/index');
const fs = require('fs');
const expect = require('chai').expect;

var Excel = verquire('excel');
const Excel = verquire('excel');

var TEST_FILE_NAME = './spec/out/wb.test.xlsx';
const TEST_FILE_NAME = './spec/out/wb.test.xlsx';

// need some architectural changes to make stream read work properly
// because of: shared strings, sheet names, etc are not read in guaranteed order
describe('WorkbookReader', function() {
describe('Serialise', function() {
it('xlsx file', function() {
this.timeout(10000);
var wb = testutils.createTestBook(new Excel.Workbook(), 'xlsx');
const wb = testutils.createTestBook(new Excel.Workbook(), 'xlsx');

return wb.xlsx.writeFile(TEST_FILE_NAME)
.then(function() {
Expand All @@ -27,7 +27,7 @@ describe('WorkbookReader', function() {
describe('#readFile', function() {
describe('Row limit', function() {
it('should bail out if the file contains more rows than the limit', function() {
var workbook = new Excel.Workbook();
const workbook = new Excel.Workbook();
// The Fibonacci sheet has 19 rows
return workbook.xlsx.readFile('./spec/integration/data/fibonacci.xlsx', {maxRows: 10})
.then(function() {
Expand All @@ -39,7 +39,7 @@ describe('WorkbookReader', function() {

it('should fail fast on a huge file', function() {
this.timeout(20000);
var workbook = new Excel.Workbook();
const workbook = new Excel.Workbook();
return workbook.xlsx.readFile('./spec/integration/data/huge.xlsx', {maxRows: 100})
.then(function() {
throw new Error('Promise unexpectedly fulfilled');
Expand All @@ -49,7 +49,7 @@ describe('WorkbookReader', function() {
});

it('should parse fine if the limit is not exceeded', function() {
var workbook = new Excel.Workbook();
const workbook = new Excel.Workbook();
return workbook.xlsx.readFile('./spec/integration/data/fibonacci.xlsx', {maxRows: 20});
});
});
Expand All @@ -58,7 +58,7 @@ describe('WorkbookReader', function() {
describe('#read', function() {
describe('Row limit', function() {
it('should bail out if the file contains more rows than the limit', function() {
var workbook = new Excel.Workbook();
const workbook = new Excel.Workbook();
// The Fibonacci sheet has 19 rows
return workbook.xlsx.read(fs.createReadStream('./spec/integration/data/fibonacci.xlsx'), {maxRows: 10})
.then(function() {
Expand All @@ -69,16 +69,39 @@ describe('WorkbookReader', function() {
});

it('should parse fine if the limit is not exceeded', function() {
var workbook = new Excel.Workbook();
const workbook = new Excel.Workbook();
return workbook.xlsx.read(fs.createReadStream('./spec/integration/data/fibonacci.xlsx'), {maxRows: 20});
});
});
});

describe('edit styles in existing file', function(){
beforeEach(function(){
Copy link
Member

@alubbe alubbe Jan 21, 2019

Choose a reason for hiding this comment

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

why? I quite like moving things to async/await. the test suite runs on node, so there's no transpiling here

Copy link
Member Author

@Siemienik Siemienik Jan 21, 2019

Choose a reason for hiding this comment

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

we currently support node v6 :(
https://travis-ci.org/exceljs/exceljs/builds/479501545?utm_source=github_status&utm_medium=notification

Copy link
Member

@alubbe alubbe Jan 21, 2019

Choose a reason for hiding this comment

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

damn, right you are :(

this.wb = new Excel.Workbook();
return this.wb.xlsx.readFile('./spec/integration/data/test-row-styles.xlsx');
});

it('edit styles of single row instead of all', function () {
const ws = this.wb.getWorksheet(1);

ws.eachRow((row, rowNo) => {
rowNo % 5 === 0 && (row.font = {color: {argb: '00ff00'}})
});

expect(ws.getRow(3).font.color.argb).to.be.equal(ws.getRow(6).font.color.argb);
expect(ws.getRow(6).font.color.argb).to.be.equal(ws.getRow(9).font.color.argb);
expect(ws.getRow(9).font.color.argb).to.be.equal(ws.getRow(12).font.color.argb);
expect(ws.getRow(12).font.color.argb).not.to.be.equal(ws.getRow(15).font.color.argb);
expect(ws.getRow(15).font.color.argb).not.to.be.equal(ws.getRow(18).font.color.argb);
expect(ws.getRow(15).font.color.argb).to.be.equal(ws.getRow(10).font.color.argb);
expect(ws.getRow(10).font.color.argb).to.be.equal(ws.getRow(5).font.color.argb);
})
});

describe('with a spreadsheet that contains formulas', function() {
before(function() {
var testContext = this;
var workbook = new Excel.Workbook();
const testContext = this;
const workbook = new Excel.Workbook();
return workbook.xlsx.read(fs.createReadStream('./spec/integration/data/formulas.xlsx'))
.then(function() {
testContext.worksheet = workbook.getWorksheet();
Expand Down Expand Up @@ -130,8 +153,8 @@ describe('WorkbookReader', function() {

describe('with a spreadsheet that contains a shared string with an escaped underscore', function() {
before(function() {
var testContext = this;
var workbook = new Excel.Workbook();
const testContext = this;
const workbook = new Excel.Workbook();
return workbook.xlsx.read(fs.createReadStream('./spec/integration/data/shared_string_with_escape.xlsx'))
.then(function() {
testContext.worksheet = workbook.getWorksheet();
Expand Down