Skip to content

Comments

refactor: use neoqs and vitest#268

Open
YurySolovyov wants to merge 2 commits intobalazsbotond:masterfrom
YurySolovyov:neoqs
Open

refactor: use neoqs and vitest#268
YurySolovyov wants to merge 2 commits intobalazsbotond:masterfrom
YurySolovyov:neoqs

Conversation

@YurySolovyov
Copy link

Summary

Fixes #267

Details

I couldn't get jest to work, so I converted to vitest which works for me, but I can revert if that unacceptable

@@ -1,9 +1,9 @@
import qs, { IStringifyOptions } from 'qs';
import { stringify, StringifyOptions } from 'neoqs';
Copy link

Choose a reason for hiding this comment

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

Should be neoqs/legacy, as neoqs doesn't support CJS, but legacy subpath does

Copy link
Author

Choose a reason for hiding this comment

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

why would that matter? my understanding is it's getting built into both esm and cjs by tsup from esm source

Copy link

Choose a reason for hiding this comment

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

Im not sure if current setup builds qs also in, based on my experience it prolly doesn't.

If it builds it in then its good, otherwise it should be switched to /legacy

Copy link
Author

Choose a reason for hiding this comment

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

yes, I wish CI would trigger, but locally all tests pass

Copy link

Choose a reason for hiding this comment

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

tests won't catch this issue. Can you locally build and inspect whether the output file is importing qs or is it fully inlined?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! Added noExternal from: https://www.jsdocs.io/package/tsup#Options

/** Always bundle modules matching given patterns */
noExternal?: (string | RegExp)[];

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.

Consider alternative query sting package

2 participants