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

Make it an NPM package. #51

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dwjohnston
Copy link

@dwjohnston dwjohnston commented Mar 29, 2022

Addresses: #42

Changes:

  • I've moved the core of the logic out to stix2viscore.js. Retain stix2vis for compatibility with what you already have.
  • Adds a package.json
  • Adds linting - I mostly added this to help me remove all the non-strict js.
  • Adds typescript as a dependency - I'm using TypeScript to compile - but currently I haven't added types, but this would be a very nice feature feature.
  • Add package scripts (allow you to see the browser, build command)

Notes

Re: the use of requirejs, and the "nbextensions" stuff - I don't really know what I'm doing there - but what I've got looks like it's working.

Possible bug

Take a look at that handleSelected in stix2viscore.js - it looks like it is never defined anywhere.

Worth discussing

  • Should the PNG be included in the package?
  • With putting all of the code in stix2viscore.js you will lose your commit history. Possibly better to put the Jupyter stuff in a different file.
  • no-prototype-builtins - I just disabled the rule - is this something worth fixing? https://eslint.org/docs/rules/no-prototype-builtins

Testing

I tested that it works as an npm package here:

https://github.com/dwjohnston/stix-vis-test

Follow the instructions there.

Things that need to be done before this PR is merged:

  • Update the package.json
    • Rename name
    • Repository
    • Author
    • License
  • Update the readme
    • Address TODOS
    • Include installation instructions
    • (Because you might as well) - Include example usage in react

Future improvements:

  • Add types

@dwjohnston dwjohnston marked this pull request as ready for review March 29, 2022 06:00
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.

1 participant