rspieker / jest-transform-svelte

Jest Transformer for Svelte components
35 stars 7 forks source link

Use Svelte compiler instead of Rollup, add Pre-processor support #6

Closed will-wow closed 5 years ago

will-wow commented 5 years ago

Hi! Thanks so much for putting together this transformer! It was great for getting going with Svelte-Testing-Library right away.

But when I added TypeScript to my svelte project, I ran into a couple issues writing tests with this transformer:

  1. When testing a svelte component that imported non-svelte modules, I was getting errors because I was relying on rollup plugins that weren't getting run in jest-transformer-svelte, like includepaths for absolute imports
  2. I had to do import x from './x.ts, instead of import x from './x, because the jest-transformer-svelte rollup config didn't know about my registered extensions with the resolve plugin
  3. I couldn't use svelte-preprocess to make TypeScript svelte components

Proposed Solution

Svelte Compiler

Looking around the internet, I've seen other transform plugins using the svelte compiler directly to emit CommonJS that Jest can understand, no rollup required.

Here's the one I cribbed from:

https://github.com/WeAreGenki/minna-ui/blob/e2bedb0ec6efc652756bde9fa036d59e42f8e8ed/utils/jest-config/transforms/svelte.js#L33

That transformer also includes a solve for a problem described in https://github.com/sveltejs/svelte/issues/2562#issuecomment-489251804, where the cjs the compiler emits makes you do

import Component from './Component.svelte`

new Component.default()

which is lame.

PeerDependency

I moved svelte from a depenceny to a peerDependency, so it will use whatever version of the svelte compiler the user's project is on.

Pre-processors

This PR also includes letting the user pass in pre-processor configuration to the transformer, which lets people test components that are preprocessed (for typescript, or coffeescript, or whatever)

For Discussion

First of all, I know this is a big unsolicited change, so if you're using Rollup for other reasons, then no hard feelings if you want to close this PR. But Svelte-Testing-Library links to this repo, so I figured this would be the best place to propose the change.

Also, I added the typescript pre-processor to the example project, and everything seems to be working. But are there other ways we could test this change to make sure it doesn't break anything?

Thanks again, and I hope this is helpful!

RobbieTheWagner commented 5 years ago

This sounds awesome! About to try it out now 😃

RobbieTheWagner commented 5 years ago

@will-wow I have been trying hard to get my tests running, but I seem to always get ParseError: Identifier is expected. Any ideas how to fix that?

will-wow commented 5 years ago

@rwwagner90 thanks for trying it out!

And hm, that sounds like something isn't getting pre-processed before it gets to jest. I did just notice that my node_modules must have been in a weird state when I was developing this, because the example/package.json is pointing at jest-transform-svelte: 2.0.0 instead of jest-transform-svelte: "file:..". Fixed that, and so running npm test seems to be working in the example directory now.

But assuming you're trying this on your own project, what preprocessors are you using? I just added a little debug option. Pass {debug: true} to the transformer and run jest --no-cache, and it'll print out the compiled code being sent to Jest. If you see some un-preprocessed code, then that's a clue!

RobbieTheWagner commented 5 years ago

@will-wow this is where I am trying to use it https://github.com/shipshapecode/shepherd/pull/551

Perhaps you could take a quick look and see if anything looks wrong?

will-wow commented 5 years ago

@rwwagner90 well that's odd. When I first ran yarn test on your project, I saw those same errors, but on every other run only two tests are failing, which seem unrelated to svelte 😖 .

I am also seeing an occasional weird error from node-sass. Since I'm guessing most people just want to ignore they style tags anyway in a jest test, I added something I saw here: https://github.com/WeAreGenki/minna-ui/blob/e2bedb0ec6efc652756bde9fa036d59e42f8e8ed/utils/jest-config/transforms/svelte.js#L22 that strips out style tags entirely.

Maybe that'll help? You can use

    "jest-transform-svelte": "will-wow/jest-transform-svelte#75466a226e5fd09e28c991cd1a29cf3338966b19",

to try it.

RobbieTheWagner commented 5 years ago

well that's odd. When I first ran yarn test on your project, I saw those same errors, but on every other run only two tests are failing, which seem unrelated to svelte 😖 .

What node/yarn etc versions are you on? I wish I could get it to work after the first run like that!

Yeah, there should be some failures, I wasn't able to see what was failing and fix it, since I could not run the tests.

I will try this new option and see if it helps.

RobbieTheWagner commented 5 years ago

@will-wow that option works! However, I would actually like to write some tests that are style dependent potentially on occasion. I wonder if we can compare our environments to determine what is different and get keeping the styles working?

rspieker commented 5 years ago

Hey @will-wow and @rwwagner90, thank you for contributing to this module! I'm currently not all that well versed in TypeScript so I'm going to dedicate some time and see what the PR does and whether the proposal can benefit both TypeScript and JavaScript developers

RobbieTheWagner commented 5 years ago

@rspieker this isn't for TS, this is for allowing preprocessing with svelte. If you do not do the preprocessing, the transforms and jest tests do not run.

will-wow commented 5 years ago

Hey @rspieker thanks! Yeah this happens to work for typescript, but it should help with any preprocessor, and also should help with issues like https://github.com/rspieker/jest-transform-svelte/issues/4 (because it will let Jest handle imports instead of going through rollup) and maybe https://github.com/rspieker/jest-transform-svelte/issues/7 (I think if rollup is doing the imports then Jest doesn't have a chance to inject mocks)

will-wow commented 5 years ago

And @rwwagner90 I was on node 12.6.0 and yarn 1.17.3. Also if you're not already, I'd suggest trying jest --no-cache when debugging issues, since that seems to make errors at least happen more consistently.

And yeah I can see wanting to have styles in a test. My guess is that deasync isn't playing nicely with node-sass, which is causing intermittent failures. But I'm not sure what to do about that, since Svelte preprocessors are async but jest transforms have to be sync.

Any luck getting things working?

RobbieTheWagner commented 5 years ago

@will-wow do you think it would help if I used postcss with some plugins for nested etc instead of scss?

will-wow commented 5 years ago

@rwwagner90 afaik postcss is pure JavaScipt (unlike the C bindings with node-sass), so yeah I could see it would working better with deasync. I'd certainly be interested in finding out, if you want to try it on a component and report back!

I'm also curious, what's your use case for rendering components with styles? are you thinking of doing visual regression testing or something?

rspieker commented 5 years ago

Merged the PR, thanks for all the effort you've put in!

rspieker commented 5 years ago

@will-wow Do you mind if I add you as a contributor to the jest-transform-svelte module?

And if you don't mind; is this the info you want added?

"contributors": [
    "Will <wow@carbonfive.com> (https://github.com/will-wow)"
]
RobbieTheWagner commented 5 years ago

@will-wow I will try postcss and see if I get better results.

I'm using this for https://github.com/shipshapecode/shepherd so my use case is we might want to check some styles in some test, to ensure we ship the right ones. We do not currently do this, but would be nice to.

RobbieTheWagner commented 5 years ago

I ultimately decided just to use normal CSS, as we don't have a ton of styles, so things work again. Thanks everyone! 😃

will-wow commented 5 years ago

@rspieker sure, I'd love to be added as a contributor, thanks! And yeah that info is correct.

I'm glad this PR was helpful, and thanks for testing it out @rwwagner90!