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

Improvements to javascript and build process #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

paustint
Copy link

@paustint paustint commented Jan 8, 2023

PR Details

  • Made minor updates to index.js
  • Added a test (just for nodejs)
  • Added package.json scripts to automate build process

Description

  • Added prettier for consistent code formatting and applied formatting. (This accounts for most of the code changes)
  • Updated javascript to simplify some of the environment detection
  • Moved import of pako to top of file
  • Ensure that performance object in node is not fully replaced and instead just now is replaced
  • Added scripts to package.json for building and testing.
  • Added source maps - this is nice for library consumers that need to debug
  • Updated compatibility chart. globalThis bumped node from 8 to 12.
  • Updated build process to automatically include type definition

Related Issue

There is no open issue for this, since it is not really a feature or a bug. Happy to discuss changes here or open an issue if we would like to track an issue along with this PR.

Please let me know. 🙏

Motivation and Context

The init function was using a mix of legacy JS and modern JS. This PR cleans up these minor issues.
The build process was not documented or automated, this PR includes documentation in the README and automates this using package.json scripts.

In addition, a test has been added to ensure that the built code works.

How Has This Been Tested

I added a file to test in NodeJS (just basic usage for now) and I have tested manually in a Typescript web project to ensure that the changes work correctly.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
    • I don't see standards documented, but I added prettier that should handle the formatting part of the style
  • My change requires a change to the documentation.
    • Readme was updated, but core docs do not need to be updated
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
    We still have a lot of tests that would benefit from being written

Added prettier for consistent code formatting and applied formatting.
(This accounts for most of the code changes)

Updated javascript to simplify some of the environment detection

Moved import of pako to top of file

Ensure that performance object in node is not fully replaced
and instead just `now` is replaced

Added scripts to package.json for building and testing.

Added source maps - this is nice for library consumers that need to debug

Updated compatibility chart. globalThis bumped node from 8 to 12.

Updated build process to automatically include type definition
Comment on lines +46 to +51
{
// Include type definitions
input: "./src/index.d.ts",
output: [{ file: "dist/index.d.ts", format: "es" }],
plugins: [dts()],
},
Copy link
Author

Choose a reason for hiding this comment

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

This was the only actual change to this file, everything else was just a formatting change from prettier

Comment on lines +20 to +26
"scripts": {
"build": "npm-run-all clean build:js build:go",
"build:js": "rollup -c",
"build:go": "cd cmd && GOOS=js GOARCH=wasm go build -ldflags=\"-s -w\" -o ../dist/excelize.wasm && cd ../dist && gzip -9 -v -c excelize.wasm > excelize.wasm.gz && rm excelize.wasm",
"clean": "rm -rf dist",
"test": "node test/test-nodejs.js"
},
Copy link
Author

Choose a reason for hiding this comment

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

Any of these scripts can be executed by running npm run x - e.x. npm run build.
npm-run-all is a library that allows running multiple npm commands without having to chain them together.

gzip -9 -v -c excelize.wasm > excelize.wasm.gz && rm excelize.wasm is probably not cross-platform compatible, likely won't work on Windows.

@paustint
Copy link
Author

paustint commented Jan 8, 2023

@xuri - Hopefully I did not overstep any boundaries by raising this PR and I hope you find the changes valuable.
I am happy to discuss any of the changes proposed in the PR. Thank you 😸

@xuri
Copy link
Owner

xuri commented Jan 13, 2023

Thanks for your pull request. This PR contains a lot of code and I need some time to review.

@paustint
Copy link
Author

@xuri - No problem, take your time. Let me know if you have any questions or if there is anything that I came up with that you disagree with.

Thanks!

@xuri xuri added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants