-
Notifications
You must be signed in to change notification settings - Fork 12
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
BREAKING CHANGE: Migrate to Jest and Webpack to be more compatible with Lit 3 #118
Conversation
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.
a couple of nits but otherwise lgtm!
@@ -3,7 +3,8 @@ | |||
"parserOptions": { | |||
"ecmaFeatures": { | |||
"jsx": true | |||
} | |||
}, | |||
"ecmaVersion": 2018 |
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.
would we want the latest version?
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.
Unfortunately, the last version this code works with is the 2018
@@ -1,7 +1,7 @@ | |||
'use strict'; | |||
|
|||
var React = require('react'), //eslint-disable-line no-unused-vars | |||
TestUtils = require( 'react-addons-test-utils' ), | |||
var React = require('react'), //eslint-disable-line no-unused- |
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.
nit: is //eslint-disable-line no-unused-
still required?
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 👍
@@ -41,6 +41,7 @@ describe('Generic Viewer', function() { | |||
|
|||
it('should render wrapper with expected class name', function() { | |||
var elem = TestUtils.renderIntoDocument( | |||
|
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.
nit: Is this newline intentionally added?
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.
Unintentionally 💯
@@ -18,7 +18,7 @@ describe('Image Plugin', function() { | |||
}); | |||
|
|||
it('should return a view', function() { | |||
var viewer = ImagePlugin.getComponent(); | |||
var viewer = ImagePlugin.getComponent({src: 'test'}); |
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.
For the test in src/plugins/html/__tests__/htmlPluginTests.js
, the file ends in .html
, i.e. src: 'test.html'
, should this have the extension too, or perhaps the other test shouldn't have it? 🤔
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.
Thx, any value will do here, so I removed the .html for consistency
jest.config.js
Outdated
// ORIGINAL COMMENT FROM REMOVED KARMA CONFIG: | ||
// There's a pre-test stall on Travis builds that can cause Karma to | ||
// fail with the default timeout of 10s. I'm not sure what's causing | ||
// the stall, but this mitigates it for now. | ||
// | ||
// KEEPING THE TIMEOUT JUST IN CASE |
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.
We don't use Travis for builds anymore (should be GitHub Actions), so I wonder if the comment/timeout is still relevant 🤔
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
@@ -10,7 +10,7 @@ describe('Generic Plugin', function() { | |||
}); | |||
|
|||
it('should return a view', function() { | |||
var viewer = GenericPlugin.getComponent(); | |||
var viewer = GenericPlugin.getComponent({src: 'test'}); |
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.
Same comment as for src/plugins/image/__tests__/imagePluginTests.js
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.
Out of curiosity, what led to this var name change from native
to plugin
?
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.
It's weird, but test fails with ReferenceError: native is not defined
if we name this variable differently from exported here
module.exports = plugin; |
…th Lit 3 (#118) BREAKING CHANGE: Migrate to Jest and Webpack to be more compatible with Lit 3
🎉 This PR is included in version 3.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Related jira:
https://desire2learn.atlassian.net/browse/PHNX-1954
This PR:
Karma, Browserify, Babelify
withJest, Webpack
.bump-react-valence-ui-iframe
.Why do we need this?
While configuring the
Babelify
for Lit 3, we had to update some dependencies that bring along others. As a result, we arrive at a configuration where we can no longer stably work withReact
versions earlier than16
.Why Jest?
Jest is perfectly compatible with React. It debugs well and doesn't require a bundler to execute tests.