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

modernize build test and lint #185

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

Conversation

chris-armstrong
Copy link

This is a PR to modernize the build and test infrastructure, including:

  • using preconstruct to generate ESM and commonjs compatible exports for bundling
  • removing the use of gulp and replacing with npm scripts
  • upgrading packages to address security warnings

@whitlockjc
Copy link
Owner

First off, thanks a ton for taking this on. I'll review this more in depth ASAP.

Copy link
Owner

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

It looks like the majority of this code was to move to using export and import keywords, and a lot of other changes were a result of this. I'm fine with evolution, but I'd really like to understand why and also to make sure we're making only necessary changes.

bin/json-refs Show resolved Hide resolved
dist/json-refs.cjs.js Outdated Show resolved Hide resolved
dist/json-refs.esm.js Outdated Show resolved Hide resolved
dist/tester.html Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
dist/json-refs.cjs.js Outdated Show resolved Hide resolved
@chris-armstrong
Copy link
Author

I should note that I'm no longer depending on these changes to make further additions, so I won't be chasing these up, but they may be useful to anyone consuming the library or submitting PRs.

@whitlockjc
Copy link
Owner

@chris-armstrong Thanks again for doing this, and if you're still willing to help me get this ready to merge, I'd love to use it. I know you said you don't depend on this anymore but I think the changes you're suggesting could be welcome. Where does this PR stand in your opinion? Also, do you mind updating CONTRIBUTING.md to reflect the new build/release process?

@chris-armstrong
Copy link
Author

I don't have anything I want to add to this PR, but because it's been a couple of months, and I'm not using the changes, I think best next step is to verify it hasn't regressed for downstream consumers of the module.

I'd say the next steps is to:

  • resolve outstanding merge conflicts
  • build and release as an alpha version on npm so that it can be upgraded in downstream projects to test compatibility
  • update CONTRIBUTING.md to reflect new build process (npm run build)

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.

2 participants