-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Use rollup for browser builds #3532
Conversation
scripts/build.js
Outdated
if (browser.indexOf(BUILD_ES5_DIR) !== 0) { | ||
throw new Error( | ||
`browser field for ${pkgJsonPath} should start with "${BUILD_ES5_DIR}"` | ||
async function buildBrowserPackages(packages) { |
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.
Using async/await
and for of
loops here in order for getting a nice stdout
output similar to the node builds.
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.
The current appveyor
with node 6 doesn't like this... So should we upgrade the nodejs_version
to 7 or should I just get rid of async/await?
async function buildBrowserPackages(packages) {
^^^^^^^^
SyntaxError: Unexpected token function
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 just removed it...
3c45bcc
to
d3ed28b
Compare
Codecov Report
@@ Coverage Diff @@
## master #3532 +/- ##
==========================================
- Coverage 58.09% 58.07% -0.03%
==========================================
Files 195 195
Lines 6751 6747 -4
Branches 6 6
==========================================
- Hits 3922 3918 -4
Misses 2826 2826
Partials 3 3
Continue to review full report at Codecov.
|
Nice! I don't know how much work adding a few browser tests (using mocha, along with jest matchers and jest mocks?) running with karma or something would be? I think that needs to be included in the Jest repo to avoid unknowingly breaking it in the future. |
Setting up the tests is rather easy, but we need infrastructure to run the tests on a CI. |
For example sauce labs or browser stack. A docker image with a headless browser is also a possibility. Not sure if Jests CI supports that. : ) |
@skovhus Travis has some recommendations for headless testing that can be done without another third party service: https://docs.travis-ci.com/user/gui-and-headless-browsers/#Using-xvfb-to-Run-Tests-That-Require-a-GUI |
d3ed28b
to
c54e4d0
Compare
@probablyup @SimenB I added some Karma based tests... Works on CI! 👍 Any thoughts/wishes on where to place these tests? And do you want a separate package.json to contain all the karma related things? Another things: I removed some of the browser/es5 builds that I added earlier. I think it might be enough for now to support |
4268760
to
5861ddb
Compare
@aaronabramov, a few open questions:
|
No idea how I lost track of this. Awesome that you were able to add a browser test! Really looking forward to replacing chai and sinon in the browser tests we've got at work. |
|
||
it('runs mocks', function() { | ||
var someMockFunction = mock.fn(); | ||
expect(someMockFunction.mock.calls.length).toBe(0); |
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.
expect(someMockFunction).not.toHaveBeenCalled();
?
Do you mind rebasing this one? |
503a03d
to
49ddf7a
Compare
@cpojer sure... I gave it a quick try, but #3778 gives me some issues do to some bable configuration.
Will look more into this at some point. But ideas are more than welcome @SimenB Also let me know your thoughts on the two questions above (#3532 (comment)). |
Hmm, we might have to convert to Started on this, but it doesn't really scale: diff --git i/scripts/browserBuild.js w/scripts/browserBuild.js
index e0c6a5ee..570d5630 100644
--- i/scripts/browserBuild.js
+++ w/scripts/browserBuild.js
@@ -47,7 +47,31 @@ function browserBuild(pkgName, entryPath, destination) {
plugins: [
rollupFlow(),
rollupJson(),
- rollupCommonjs(),
+ rollupCommonjs({
+ namedExports: {
+ 'packages/jest-matchers/src/jasmine_utils.js': [
+ 'equals',
+ 'fnNameFor',
+ 'hasProperty',
+ 'isA',
+ 'isUndefined',
+ ],
+ 'packages/jest-matchers/src/jest_matchers_object.js': [
+ 'getMatchers',
+ 'getState',
+ 'setMatchers',
+ 'setState',
+ ],
+ },
+ }),
rollupBabel(babelEs5Options),
rollupGlobals(),
rollupBuiltins(), |
scripts/build.js
Outdated
p.split('/').pop(), | ||
path.resolve(srcDir, 'index.js'), | ||
path.resolve(p, browser) | ||
).then(() => { |
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.
can you add a catch
here?
(node:21659) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: 'getMatchers' is not exported by packages/jest-matchers/src/jest_matchers_object.js
vs
{ Error: 'getMatchers' is not exported by packages/jest-matchers/src/jest_matchers_object.js
at error (/Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:170:12)
at Module.error$1 [as error] (/Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:8007:2)
at Module.trace (/Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:8106:9)
at ModuleScope.findDeclaration (/Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:7691:22)
at Scope.findDeclaration (/Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:5497:39)
at CallExpression.bind (/Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:6022:28)
at /Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:5291:50
at VariableDeclarator.eachChild (/Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:5308:5)
at VariableDeclarator.bind (/Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:5291:7)
at /Users/simbekkh/repos/jest/node_modules/rollup/dist/rollup.js:5291:50
code: 'MISSING_EXPORT',
url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module',
pos: 1321,
loc:
{ file: '/Users/simbekkh/repos/jest/packages/jest-matchers/src/index.js',
line: 31,
column: 29 },
frame: '29: import { equals } from \'./jasmine_utils\';\n30: import { any, anything, arrayContaining, objectContaining, stringContaining, stringMatching } from \'./asymmetric_matchers\';\n31: import { getState, setState, getMatchers, se
tMatchers } from \'./jest_matchers_object\';\n ^\n32: import extractExpectedAssertionsErrors from \'./extract_expected_assertions_errors\';' }
diff --git i/scripts/build.js w/scripts/build.js
index 5f73065c..88d084ce 100644
--- i/scripts/build.js
+++ w/scripts/build.js
@@ -94,7 +94,11 @@ function buildBrowserPackage(p) {
).then(() => {
process.stdout.write(adjustToTerminalWidth(`${path.basename(p)}\n`));
process.stdout.write(`${OK}\n`);
- });
+ })
+ .catch(e => {
+ console.error(e);
+ process.exit(1);
+ });
}
}
var someMockFunction = mock.fn(); | ||
expect(someMockFunction.mock.calls.length).toBe(0); | ||
someMockFunction(); | ||
expect(someMockFunction.mock.calls.length).toBe(1); |
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.
expect(someMockFunction).toHaveBeenCalled();
or expect(someMockFunction).toHaveBeenCalledTimes(1);
I prefer not to use exports for now as this would be another large codemod. Could we use it only for jest-matcher-utils for now and its dependencies? I'd prefer it if there was a way to make it work with current ES module imports that don't use babel as well – is there such a plugin for babel except the default one? |
"It" as in |
|
17179c8
to
7eeced6
Compare
@@ -69,4 +69,4 @@ const extractExpectedAssertionsErrors = () => { | |||
return result; | |||
}; | |||
|
|||
module.exports = extractExpectedAssertionsErrors; | |||
export default extractExpectedAssertionsErrors; |
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.
@cpojer what this what you proposed? : )
7eeced6
to
411c89e
Compare
Awesome. |
Published |
Thanks for merging! Exciting 🌟 Let us try it in the wild! |
@cpojer I guess once this is tested and we are happy, then the documentation should be updated. |
Yes, I'd like to try this out, but I'm not sure where to start 😅 |
😉 |
@skorvus, I lack some context here, could you help me out? The result is that rollup browser builds don't transpile ES6 constructs correctly
Any idea why running Technically the only difference is that symlinks to other workspaces are not in |
Let us discuss this in your PR. |
* Remove browser builds for sub dependencies * Add karma browser tests for es5 builds * Use rollup for browser builds * Review comment + prettier * Fix rollup build by using export for jest-matchers * Run prettier on unrelated file
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. |
Summary
In order to provide browser versions of relevant Jest packages (#3360) we've tried:
build-es5
infrastructure using babel)Here we actually do a proper rollup build including all dependencies and handle packages expecting node globals like
process
. Similar to whatprettier
does (https://github.com/prettier/prettier/blob/master/docs/rollup.config.js).Test plan
I've added an automated test that runs these packages in a browser.
After merging, I would like Jest beta build, so I can test this on the repos I'm trying to convert to Jest + Expect using
jest-codemods
. skovhus/jest-codemods#39