-
Notifications
You must be signed in to change notification settings - Fork 33
feat: move the module to be esm #221
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
Conversation
To do that I've also switched jest for vitest **NOTE** I'm soon going to be potentially without Internet connection. I don't want to merge this and just walk away, so I'll need some :+1: and backup peeps on this one.
c15f692 to
313c11e
Compare
fwiw, it works for me (once I installed @testing-library/dom)
@cesarnml can you try this branch for #220 and see if it solves the problem?
I attempted to apply the patched branch "@testing-library/svelte": "github:yanick/svelte-testing-library", and installed @testing-library/dom, but now I see a new error:
FAIL src/routes/page.spec.ts [ src/routes/page.spec.ts ]
Error: Failed to resolve entry for package "@testing-library/svelte". The package may have incorrect main/module/exports specified in its package.json.
❯ packageEntryFailure node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:23382:11
❯ resolvePackageEntry node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:23379:5
❯ tryNodeResolve node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:23113:20
❯ Context.resolveId node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:22874:28
❯ Object.resolveId node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:42847:46
❯ TransformContext.resolve node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:42575:23
❯ normalizeUrl node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:40500:34
❯ async file:/Users/cesar/code/svelte4-testing-example/node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:40651:47
❯ TransformContext.transform node_modules/.pnpm/vite@4.3.9_@types+node@20.2.5/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:40577:13
Full disclosure.
Module resolution errors are not my cup of tea, so it's perfectly possible that I don't know how to apply the fix properly. Open to suggestions here.
Repo with patched branch:
https://github.com/cesarnml/svelte4-testing-example/blob/759b0999bb8f840f97864bf930707eb69e30c442/package.json#L20
Steps I took from minimal example repo to patch repo:
pnpm i -D yanick/svelte-testing-library
pnpm i -D @testing-library/dom
pnpm test:unit
Let me know if I goofed on applying the fix @yanick
Okay. I just realized I need to target without-babel branch above.
The fix works for me too!
Thank you for the quick turn around time. Only other suggestion I have is to perhaps update the peer dependency designation wrt svelte:
WARN Issues with peer dependencies found
.
└─┬ @testing-library/svelte 0.0.0-semantically-released
└── ✕ unmet peer svelte@3.x: found 4.0.0-next.0
working and patched example:
https://github.com/cesarnml/svelte4-testing-example/blob/1f7ab0768510495fcb5c317b5ae769902bf777c4/package.json#L41
Update:
Patch also works on my production svelte-kit app repo I'm working on. 🎉 🙏 . Ship it 😄
https://github.com/cesarnml/waka-shortcut-time-stats
just realized I need to target
without-babelbranch above.
Heh. You fixed it as I was finding the same thing. ^.^
The fix works for me too! Thank you for the quick turn around time. Only other suggestion I have is to perhaps update the peer dependency designation wrt svelte:
Yeah, we should probably do that once svelte 4.0.0 gets out of alpha.
Update: Patch also works on my production svelte-kit app repo I'm working on. tada pray . Ship it smile https://github.com/cesarnml/waka-shortcut-time-stats
Woo! Now, I'm about to be offline for a few days, so I won't do the mistake of merging and going dark. :turn to the whole room: Do we have another maintainer who can check this PR and, if we merge, keep eyes on the status of things for the next few days?
Update!: Spoke too soon. Found this error on my production branch above.
Error: No "exports" main defined in /Users/cesar/code/waka-shortcut-time-stats/node_modules/.pnpm/@testing-library+svelte@3.2.2_svelte@4.0.0-next.0/node_modules/svelte/package.json
❯ Object.<anonymous> node_modules/.pnpm/@testing-library+svelte@3.2.2_svelte@4.0.0-next.0/node_modules/@testing-library/svelte/dist/pure.js:28:15
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: {
"code": "ERR_PACKAGE_PATH_NOT_EXPORTED",
}
Could be from the pure.js in the root directory. I tweaked it, try again?
cesarnml
commented
Jun 7, 2023
Could be from the pure.js in the root directory. I tweaked it, try again?
Test Files 31 passed (31)
Tests 60 passed (60)
Start at 10:20:19
Duration 12.71s (transform 1.39s, setup 12.87s, collect 11.49s, tests 1.44s, environment 10.89s, prepare 2.36s)
% Coverage report from c8
Success. No issues now.
Success. No issues now.
I AM A GOD! \o/
Success. No issues now.
I AM A GOD! \o/
🐐 🤣
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need jest and Vitest installed? (also jest-environment-jsdom and svelte-jester)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Leftover. Removing...
.eslintrc.cjs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this line if we're using Vitest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Maybe for the globals. Let's see ! :goes to console:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to the vitest env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does probably also need src/pure.js and src/index.js now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True!
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the older original package. there's now an eslint-plugin-svelte that you might want to migrate to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrated!
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this src/ so that things don't unexpectedly break if you add another file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point. Done!
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this since it's in src now. not sure what dont-cleanup-after-each is and whether you need that one or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed both. dont-cleanup-after-each seems to be a helper package for setting the env variable that is not used anywhere anyway.
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is no longer maintained so folks use eslint-plugin-n now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know! Done!
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still recommend putting exports in the package.json. lgtm besides that
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Svelte 4 will be released imminently. It could be good to merge this in order to avoid any bug reports
I don't have any historical familiarity with this library, but can try to take a look if any issues pop up
I don't have any historical familiarity with this library, but can try to take a look if any issues pop up
Okie. There goes nothing!
Uh oh!
There was an error while loading. Please reload this page.
To do that I've also switched jest for vitest
NOTE I'm soon going to be potentially without Internet connection. I don't want to merge this and just walk away, so I'll need some 👍 and backup peeps on this one.
Might fix #220
BREAKING CHANGE: moves the package from cjs to esm.