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

Better browser support #3421

Merged
merged 3 commits into from
May 4, 2017
Merged

Better browser support #3421

merged 3 commits into from
May 4, 2017

Conversation

skovhus
Copy link
Contributor

@skovhus skovhus commented Apr 29, 2017

Summary

This should solve #3360 by adding support for building ES2015 version of Jest packages.

I've done this in an opt in fashion. So by adding browser: "build-es5/..." to the package.json file for a package, the build step will run a babel ES2015 compilation of that package. I've started by doing this for all packages, but it slows down the build (and is not needed for half of the packages).

Test plan

  • yarn run build
  • This produces a new folder build-es5 for some of the packages. I've verified that these folder does not contain any non ES2015 code (like const declarations).
  • I've also added eslint check for untranspiled code in the build-es5 folders (https://twitter.com/kenneth_skovhus/status/851913077104799748)

I would like someone to review if more packages should be able to run in browsers.

@jest-bot
Copy link
Contributor

jest-bot commented Apr 29, 2017

Warnings
⚠️ Changes were made to package.json, but not to yarn.lock - Perhaps you need to run `yarn install`?

Generated by 🚫 dangerJS

@codecov-io
Copy link

Codecov Report

Merging #3421 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3421   +/-   ##
=======================================
  Coverage   64.04%   64.04%           
=======================================
  Files         177      177           
  Lines        6570     6570           
  Branches        4        4           
=======================================
  Hits         4208     4208           
  Misses       2361     2361           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0817d64...6176556. Read the comment docs.

@skovhus
Copy link
Contributor Author

skovhus commented May 3, 2017

Suggestion: might make sense to release this as a patch (shouldn't break anything), so we can try it out on projects wanting to use jest-matchers in older browser (e.g. remix-run/react-router#5027). : )

@cpojer
Copy link
Member

cpojer commented May 3, 2017

I'm holding off on this one right before Jest 20 but it'll be one of the first PRs after that release I'm gonna look at :)

@SimenB
Copy link
Member

SimenB commented May 3, 2017

Will it go into a 20.x release, or do we have to wait a couple of months for it? Browser compat for stub and assertion should be high, imo. It can replace chai and sinon and really take off for browser-side testing frameworks 😄
If it's not on for 20, anything we can do to put it on there?

@cpojer cpojer merged commit bd8b071 into jestjs:master May 4, 2017
@cpojer
Copy link
Member

cpojer commented May 4, 2017

Let's do it! #yolo

@skovhus what are the next steps? How close are we to renaming Jest matchers to expect?

@skovhus skovhus deleted the browser-support branch May 4, 2017 15:27
@skovhus
Copy link
Contributor Author

skovhus commented May 4, 2017

@cpojer Making a beta release (or patch) of this would help in order to test it out in older browsers. I can do that.

Not sure about the Jest Matchers tasks, but the codemod is ready whenever the test repos can build. : )

@cpojer
Copy link
Member

cpojer commented May 4, 2017

@skovhus I published jest@19.2.0-alpha.993e64af which is the latest version of master.

@skovhus
Copy link
Contributor Author

skovhus commented May 4, 2017

Cool. Let me try that out. : )

@skovhus
Copy link
Contributor Author

skovhus commented May 4, 2017

@cpojer It doesn't work yet due to regeneratorRuntime.

Chrome 47.0.2526 (Windows 10 0.0.0) A <Media> with a query that matches and a children element renders its child FAILED
	ReferenceError: regeneratorRuntime is not defined
	    at makeResolveMatcher (webpack:///~/jest-matchers/build-es5/index.js:131:9 <- tests.webpack.js:470:36)
	    at webpack:///~/jest-matchers/build-es5/index.js:82:0 <- tests.webpack.js:421:35
	    at Array.forEach (native)
	    at expect (webpack:///~/jest-matchers/build-es5/index.js:74:0 <- tests.webpack.js:413:29)
	    at Media.<anonymous> (webpack:///modules/__tests__/Media-test.js:25:17 <- tests.webpack.js:117:39)
	    at Object.ReactMount._renderSubtreeIntoContainer (webpack:///~/react-dom/lib/ReactMount.js:403:0 <- tests.webpack.js:32805:17)
	    at ReactMount.render (webpack:///~/react-dom/lib/ReactMount.js:422:0 <- tests.webpack.js:32824:24)
	    at Context.<anonymous> (webpack:///modules/__tests__/Media-test.js:24:15 <- tests.webpack.js:116:31)

See https://travis-ci.org/ReactTraining/react-media/builds/228792122?utm_source=github_status&utm_medium=notification

);
const {browser} = require(pkgJsonPath);
if (browser) {
if (browser.indexOf(BUILD_ES5_DIR) !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Can use String.prototype.startsWith

@SimenB
Copy link
Member

SimenB commented May 4, 2017

@thymikee
Copy link
Collaborator

thymikee commented May 4, 2017

It should be auto loaded by Jest

@SimenB
Copy link
Member

SimenB commented May 4, 2017

Jest isn't the runner though

@skovhus
Copy link
Contributor Author

skovhus commented May 4, 2017

I guess we should embed it to the build-es5 builds. Don't have much time to look at this right now though. : )

@thymikee
Copy link
Collaborator

thymikee commented May 4, 2017

Oh, right. In this case you need to add regenerator runtime to global scope as @SimenB said.

@skovhus
Copy link
Contributor Author

skovhus commented May 4, 2017

So nothing should be done from Jest side? Without embedding it into the jest es5 bundles the package.json "browser" field seems a bit misleading. But it might be the right solution.

@cpojer
Copy link
Member

cpojer commented May 4, 2017

Could you explicitly add regeneratorRuntime to the deps of the project you are testing and see if that unblocks you?

@thymikee
Copy link
Collaborator

thymikee commented May 4, 2017

It's already in deps, as that project uses npm 4. He needs to require/import it.

@cpojer
Copy link
Member

cpojer commented May 4, 2017

@thymikee really? Is it part of installing jest-matchers@19.2.0-alpha.993e64af?

@thymikee
Copy link
Collaborator

thymikee commented May 4, 2017

Looks like this. Apparently we use async/await syntax somewhere in jest-matchers and the regenerator is not called there (because we add it to require queue in normalization step). That project only use jest-matchers.

@skovhus
Copy link
Contributor Author

skovhus commented May 4, 2017

Adding it globally will probably work. But this isn't that friendly an interface if we want Jest-matchers to be used for testing in browsers.

@skovhus
Copy link
Contributor Author

skovhus commented May 4, 2017

Is async used a lot of places? Could consider not using it.

@cpojer
Copy link
Member

cpojer commented May 4, 2017

Totally agree, we should find a way.

@thymikee
Copy link
Collaborator

thymikee commented May 4, 2017

@skovhus there's only one place, so that's doable. We should also write an eslint rule which would forbid to use async there.

@cpojer
Copy link
Member

cpojer commented May 4, 2017

https://twitter.com/left_pad/status/860193246852648960 ?

@skovhus
Copy link
Contributor Author

skovhus commented May 5, 2017

@cpojer tried this at #3476

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Add support for building es2015 versions of packages

* Build ES2015 versions of relevant packages

* Ensure no untranspiled code by linting build-es5 folders
@github-actions
Copy link

This pull request 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants