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

PROCESS function of babel-jest returns the code-map object when config.mapCoverage is set to true #5257

Closed
gaoupon opened this issue Jan 9, 2018 · 7 comments

Comments

@gaoupon
Copy link

gaoupon commented Jan 9, 2018

Do you want to request a feature or report a bug?
request a feature

What is the current behavior?
return result.code

      const transformResult = babelTransform(src, theseOptions);
      return transformResult ? transformResult.code : src;

What is the expected behavior?
as mentioned in mapcoverage, expected return an object like this

      const transformResult = babelTransform(src, theseOptions);
      if (config.mapCoverage && transformResult) {
        return {
          code: transformResult.code || src,
          map: transformResult.map
        }
      }
      return transformResult ? transformResult.code : src;
@SimenB
Copy link
Member

SimenB commented Jan 16, 2018

There's no reason to not always return the map, is there? This diff should be enough:

diff --git i/packages/babel-jest/src/index.js w/packages/babel-jest/src/index.js
index 4dd71bac..82a40013 100644
--- i/packages/babel-jest/src/index.js
+++ w/packages/babel-jest/src/index.js
@@ -70,7 +70,7 @@ const createTransformer = (options: any) => {
     plugins: (options && options.plugins) || [],
     presets: ((options && options.presets) || []).concat([jestPreset]),
     retainLines: true,
-    sourceMaps: 'inline',
+    sourceMaps: true,
   });
   delete options.cacheDirectory;
   delete options.filename;
@@ -129,7 +129,7 @@ const createTransformer = (options: any) => {
 
       // babel v7 might return null in the case when the file has been ignored.
       const transformResult = babelTransform(src, theseOptions);
-      return transformResult ? transformResult.code : src;
+      return transformResult || src;
     },
   };
 };

sourceMaps change might be considered a breaking change, if so use both. Mind sending a PR with it, along with a test for some behavior this enables which previously didn't work?

EDIT: It probably would be a breaking change, so use 'both' (PR adding it was for debugging in vs code (#3738))

@kulshekhar
Copy link

There's no reason to not always return the map, is there?

As this would be a breaking change, there doesn't seem to be a lot of upside to doing this - from a practical perspective.

On the other hand, it (transformers returning a map) looks like a cleaner alternative. It also creates a structure for transformers to provide additional information in future should Jest decide to add features that would depend on transformers returning certain info.

One minor source of confusion could be related to how this would affect inline sourcemaps. Would they continue to be processed as they are now or will Jest require transformers to return sourcemaps separately?

@SimenB
Copy link
Member

SimenB commented Jan 17, 2018

As this would be a breaking change, there doesn't seem to be a lot of upside to doing this - from a practical perspective.

Upside is performance - looking through the source code with a regexp for an inline sourcemap, then parsing it, is heavier than just receiving it. The producer also does not have to encode the sourcemap into the source file, saving even more time.

It would be a breaking change for other users of babel-jest though, that's true...

On the other hand, it (transformers returning a map) looks like a cleaner alternative. It also creates a structure for transformers to provide additional information in future should Jest decide to add features that would depend on transformers returning certain info.

Yes, good point. Maybe some way to opt out of coverage, for instance (#5209).

One minor source of confusion could be related to how this would affect inline sourcemaps. Would they continue to be processed as they are now or will Jest require transformers to return sourcemaps separately?

Inline sourcemaps is the fallback. The way it works now is that if a .map property is returned from the transformer, we don't look for an inline sourcemap.

https://github.com/facebook/jest/blob/a441ecf635c3a89a599b7bfbb078db6163d23038/packages/jest-runtime/src/script_transformer.js#L246-L251

@kulshekhar
Copy link

Upside is performance - looking through the source code with a regexp for an inline sourcemap, then parsing it, is heavier than just receiving it. The producer also does not have to encode the sourcemap into the source file, saving even more time.

I meant the upside in introducing the breaking change by requiring transformers to return a map.

I'm not disputing the performance benefit in returning a map. If a transformer is actively maintained and updates to returning a map, it gets the performance boost. However, if a transformer is not actively maintained, this change would render it unusable with newer Jest versions. To be fair, the number of transformers is probably small enough to not care about this. But I don't know this for certain so I thought it'd be better to bring this up.

@SimenB
Copy link
Member

SimenB commented Jan 18, 2018

I meant the upside in introducing the breaking change by requiring transformers to return a map.

I didn't mean that transformers must return an object with .map, simply returning a string would still be supported. I'm saying that babel-jest can be changed to always return the object.

This part of the code would remain unchanged:

https://github.com/facebook/jest/blob/06bbe2dc51b429b74066d66d16f740ae5cf38d6d/packages/jest-runtime/src/script_transformer.js#L233-L243

@SimenB
Copy link
Member

SimenB commented Feb 11, 2018

babel-jest will start returning {code, map} after #5177 is merged. This is behind an option, so babel-jest should behave the same for consumers (but jest itself will provide that option). Rolling this issue into that.

Option will be removed in the next major version of Jest

@SimenB SimenB closed this as completed Feb 11, 2018
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants