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

Allow wasm-xlsxwriter to be used from both from web and nodejs #22

Merged
merged 24 commits into from
Dec 17, 2024

Conversation

henrikekblad
Copy link
Contributor

No description provided.

@kenkoooo kenkoooo self-requested a review November 25, 2024 16:47
@kenkoooo
Copy link
Member

Thank you for your contribution! Could you provide guidance on how we can test it to ensure it works as expected?

@henrikekblad
Copy link
Contributor Author

Updated tests to use the new web path

@henrikekblad
Copy link
Contributor Author

henrikekblad commented Nov 25, 2024

This is how I use it from the node-side:

const {
	Format,
	FormatAlign,
	RichString,
	FormatBorder,
	FormatScript,
	DocProperties,
	Color,
	Workbook
} = require('wasm-xlsxwriter');

function exportXlsx(columns, rows) {
	const workbook = new Workbook();
	const props = new DocProperties();
	props.setCompany('Test Inc.');
	workbook.setProperties(props);
	const worksheet = workbook.addWorksheet();
	worksheet.setName('Export');
	const headerStyle = new Format().setAlign(FormatAlign.Top).setAlign(FormatAlign.Left).setTextWrap().setBackgroundColor(Color.rgb(0xe2eee8)).setBorderBottom(FormatBorder.Medium).setBorderBottomColor(Color.rgb(0x0B703D)).setBorderRight(FormatBorder.Thin).setBorderRightColor(Color.rgb(0x0B703D));
	const defaultStyle = new Format().setTextWrap().setAlign(FormatAlign.Top);
	// Write header here [code removed - too complex for this example]
	// Write sheet data
	for (const [y, row] of rows.entries()) {
		for (const [x, col] of row.entries()) {
			worksheet.writeWithFormat(y + 1, x, col, defaultStyle);
		}
	}

	worksheet.setFreezePanes(1, 0);
	worksheet.autofilter(0, 0, rows.length, columns.length - 1);
	const buffer = workbook.saveToBufferSync();
	return buffer;
});

Note that no init-metod is needed.

README.md Outdated Show resolved Hide resolved
package.json Outdated
],
"license": "MIT",
"repository": {
"type": "git",
"url": "https://github.com/estie-inc/wasm-xlsxwriter"
},
"scripts": {
"build": "RUSTFLAGS='-C target-feature=+bulk-memory' wasm-pack build ./rust --target web --release && rm rust/pkg/.gitignore",
"build": "RUSTFLAGS='-C target-feature=+bulk-memory' wasm-pack build ./rust --target nodejs --out-dir pkg/nodejs --release && wasm-pack build ./rust --target web --out-dir pkg/web --release && rm rust/pkg/web/.gitignore && rm rust/pkg/nodejs/.gitignore",
Copy link
Contributor

@tamaroning tamaroning Nov 26, 2024

Choose a reason for hiding this comment

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

This should be separate script commands. It then requires changes to github workflows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess only you have access to the github workflows? Maybe it's better you do the separation after the PR has been merged?

henrikekblad and others added 2 commits November 26, 2024 13:56
Co-authored-by: tamaron <20992019+tamaroning@users.noreply.github.com>
Copy link
Member

@kenkoooo kenkoooo left a comment

Choose a reason for hiding this comment

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

It seems that we need to put more effort into supporting both browsers and Node runtimes. For example, brotli-wasm has its own build script to achieve that. The bundler target can be an option, but according to the documentation, only Webpack supports this feature at this time.

@henrikekblad
Copy link
Contributor Author

Ok, I'll follow the brotli example. They seem to be working on an inline target that may be an option in the future.

For reference:
rustwasm/wasm-pack#1334
rustwasm/wasm-bindgen#4065

@kenkoooo
Copy link
Member

It seems that your change no longer supports the web, such as Next.js. I added a test in #26 to ensure that it still works with Next.js. I hope it might be helpful.

@henrikekblad
Copy link
Contributor Author

Ok thanks, cross-env seems to fix the problem @kenkoooo.

@kenkoooo
Copy link
Member

I'm really sorry, but the CI setup was incorrect. It's already been fixed in #28. Would you mind retrying with the latest version? I apologize for the inconvenience, and thank you for your understanding.

@henrikekblad
Copy link
Contributor Author

Are you sure about the rollback @kenkoooo? I actually experienced problem with #26 when building locally without the cross-env fix.

@kenkoooo
Copy link
Member

The latest main branch passes all tests. Could you please rebase your branch onto it? Thank you.

@henrikekblad
Copy link
Contributor Author

Could this be related?

vercel/next.js#61921

@kenkoooo
Copy link
Member

I'm not entirely sure how Next.js bundles libraries, but some libraries, such as nextjs-auth0, provide both client-side and server-side functionalities.

@@ -1,5 +1,5 @@
"use client";
import initModule, { Workbook } from "wasm-xlsxwriter";
import initModule, { Workbook } from "../../../pkg.web/";
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind avoiding the use of absolute paths and using package names instead? We'd like to ensure that the package can be imported correctly. Could you please use something like wasm-xlsxwriter/client?

@henrikekblad
Copy link
Contributor Author

I cannot seem to recreate the correct package-lock-file. What npm commands did you run to get a link in the lock-file? Like this:

"node_modules/wasm-xlsxwriter": {
      "resolved": "../..",
      "link": true
 }

@henrikekblad
Copy link
Contributor Author

Ok, you guys seemed to run a newver version of npm. When I updated npm and re-created package-lock-file it seems to work.

@kenkoooo
Copy link
Member

Thank you for your excellent work! I’ll review and merge it this week.

@kenkoooo kenkoooo merged commit 91fd35c into estie-inc:main Dec 17, 2024
1 check passed
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.

3 participants