Conditionally require hammer.js #658
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…w` and `document` references.
|
We have a |
src/utils/events/event-manager.js
Outdated
|
|
||
| import { | ||
| const Manager = (typeof window === 'undefined' || typeof document === 'undefined') ? | ||
| ManagerMock : require('hammerjs').Manager; |
There was a problem hiding this comment.
We actually don't need that as well. We could just Manager set to null and have a if check when Manager object is created.
There was a problem hiding this comment.
The mock should be there so that future consumers of EventManager can call its public API without fear of failure in some environments.
There was a problem hiding this comment.
As for the isBrowser test, and src/controllers generally, that folder should be cleaned up to bring it more in-line with react-map-gl...and the globals.js file there seems to be a near-duplicate of src/utils/globals.js...thoughts on this @ibgreen @shaojingli?
For now, I can use that isBrowser.
ibgreen
approved these changes
May 24, 2017
src/utils/events/event-manager.js
Outdated
| @@ -149,3 +151,13 @@ export default class EventManager { | |||
| return event => this.manager.emit(eventAlias, event); | |||
| } | |||
| } | |||
|
|
|||
| function ManagerMock(m) { | |||
There was a problem hiding this comment.
JavaScripts "hoisting" of function names (allowing them to be used before declaration) is a mixed blessing.
I'd move this up top, so that we can see how the mocking is done in one place.
src/utils/events/event-manager.js
Outdated
|
|
||
| import { | ||
| const Manager = (typeof window === 'undefined' || typeof document === 'undefined') ? |
There was a problem hiding this comment.
We have a function in utils that is intended to do a safe check for whether we are running under node.js (a check that is not fooled e.g. by jsdom inclusion). Consider using that?
Also, add a short comment?
howtimeflies0
force-pushed
the
event-manager-hotfix
branch
from
7d575a3 to
ba70cd9
Compare
Manually hoist ManagerMock ;)
howtimeflies0
approved these changes
May 24, 2017
Firenze11
pushed a commit
to Firenze11/deck.gl
that referenced
this pull request
May 31, 2017
* Conditionally require hammer.js to avoid failure on server, on `window` and `document` references. * Use `isBrowser` for conditional requires Manually hoist ManagerMock ;)
wimrijnders
added a commit
to wimrijnders/vis
that referenced
this pull request
Jul 29, 2017
Adds mocks for certain components, in order to let module `Network` be run in unit tests. Changes: - Create a mock object for `hammer.js` when running browserless. The inspiration is taken from [here](visgl/deck.gl#658) - Create a mock function for `window.requestAnimationFrame()` when running browserless in `network/modules/CanvasRenderer.js` - Added unit test for `Network` to show that unit testing for it now works - Fixed naming of container in `test/Graph3d.test.js` Since `hammer.js` is also used in other modules, this fix is potentially an enabler for full-module unit tests for those as well.
yotamberk
pushed a commit
to almende/vis
that referenced
this pull request
Aug 8, 2017
* First working version of updating clustered edge * Added fix for #1315 as well * Enable unit testing for module Network Adds mocks for certain components, in order to let module `Network` be run in unit tests. Changes: - Create a mock object for `hammer.js` when running browserless. The inspiration is taken from [here](visgl/deck.gl#658) - Create a mock function for `window.requestAnimationFrame()` when running browserless in `network/modules/CanvasRenderer.js` - Added unit test for `Network` to show that unit testing for it now works - Fixed naming of container in `test/Graph3d.test.js` Since `hammer.js` is also used in other modules, this fix is potentially an enabler for full-module unit tests for those as well. * Cleanup unit test Network * Added unit test for fix issue #1218 * Adding test for #1315 - Interim save * Completed unit test and fixes for #1315 * Added fixes for #1291 * Added unit test for #1219 * Added header comment for Clustering.js, small fixes * Fix for unit test * Added example networks to unit test * Fix error in loading disassemblerExample * Network unit test final fixes * Fixes for linting * Fixed essential typo * Fixed linting error * Fixed unit test * Fixed unit test again
yotamberk
pushed a commit
to almende/vis
that referenced
this pull request
Aug 16, 2017
* Enable unit testing for module Network Adds mocks for certain components, in order to let module `Network` be run in unit tests. Changes: - Create a mock object for `hammer.js` when running browserless. The inspiration is taken from [here](visgl/deck.gl#658) - Create a mock function for `window.requestAnimationFrame()` when running browserless in `network/modules/CanvasRenderer.js` - Added unit test for `Network` to show that unit testing for it now works - Fixed naming of container in `test/Graph3d.test.js` Since `hammer.js` is also used in other modules, this fix is potentially an enabler for full-module unit tests for those as well. * Cleanup unit test Network * Fix for unit test * Added example networks to unit test * Fix error in loading disassemblerExample * Network unit test final fixes * Adjusted comments
primozs
pushed a commit
to primozs/vis
that referenced
this pull request
Jan 3, 2019
) * First working version of updating clustered edge * Added fix for almende#1315 as well * Enable unit testing for module Network Adds mocks for certain components, in order to let module `Network` be run in unit tests. Changes: - Create a mock object for `hammer.js` when running browserless. The inspiration is taken from [here](visgl/deck.gl#658) - Create a mock function for `window.requestAnimationFrame()` when running browserless in `network/modules/CanvasRenderer.js` - Added unit test for `Network` to show that unit testing for it now works - Fixed naming of container in `test/Graph3d.test.js` Since `hammer.js` is also used in other modules, this fix is potentially an enabler for full-module unit tests for those as well. * Cleanup unit test Network * Added unit test for fix issue almende#1218 * Adding test for almende#1315 - Interim save * Completed unit test and fixes for almende#1315 * Added fixes for almende#1291 * Added unit test for almende#1219 * Added header comment for Clustering.js, small fixes * Fix for unit test * Added example networks to unit test * Fix error in loading disassemblerExample * Network unit test final fixes * Fixes for linting * Fixed essential typo * Fixed linting error * Fixed unit test * Fixed unit test again
primozs
pushed a commit
to primozs/vis
that referenced
this pull request
Jan 3, 2019
* Enable unit testing for module Network Adds mocks for certain components, in order to let module `Network` be run in unit tests. Changes: - Create a mock object for `hammer.js` when running browserless. The inspiration is taken from [here](visgl/deck.gl#658) - Create a mock function for `window.requestAnimationFrame()` when running browserless in `network/modules/CanvasRenderer.js` - Added unit test for `Network` to show that unit testing for it now works - Fixed naming of container in `test/Graph3d.test.js` Since `hammer.js` is also used in other modules, this fix is potentially an enabler for full-module unit tests for those as well. * Cleanup unit test Network * Fix for unit test * Added example networks to unit test * Fix error in loading disassemblerExample * Network unit test final fixes * Adjusted comments
mojoaxel
pushed a commit
to visjs/vis-charts
that referenced
this pull request
Jul 12, 2019
* First working version of updating clustered edge * Added fix for #1315 as well * Enable unit testing for module Network Adds mocks for certain components, in order to let module `Network` be run in unit tests. Changes: - Create a mock object for `hammer.js` when running browserless. The inspiration is taken from [here](visgl/deck.gl#658) - Create a mock function for `window.requestAnimationFrame()` when running browserless in `network/modules/CanvasRenderer.js` - Added unit test for `Network` to show that unit testing for it now works - Fixed naming of container in `test/Graph3d.test.js` Since `hammer.js` is also used in other modules, this fix is potentially an enabler for full-module unit tests for those as well. * Cleanup unit test Network * Added unit test for fix issue #1218 * Adding test for #1315 - Interim save * Completed unit test and fixes for #1315 * Added fixes for #1291 * Added unit test for #1219 * Added header comment for Clustering.js, small fixes * Fix for unit test * Added example networks to unit test * Fix error in loading disassemblerExample * Network unit test final fixes * Fixes for linting * Fixed essential typo * Fixed linting error * Fixed unit test * Fixed unit test again
mojoaxel
pushed a commit
to visjs/vis-charts
that referenced
this pull request
Jul 12, 2019
* Enable unit testing for module Network Adds mocks for certain components, in order to let module `Network` be run in unit tests. Changes: - Create a mock object for `hammer.js` when running browserless. The inspiration is taken from [here](visgl/deck.gl#658) - Create a mock function for `window.requestAnimationFrame()` when running browserless in `network/modules/CanvasRenderer.js` - Added unit test for `Network` to show that unit testing for it now works - Fixed naming of container in `test/Graph3d.test.js` Since `hammer.js` is also used in other modules, this fix is potentially an enabler for full-module unit tests for those as well. * Cleanup unit test Network * Fix for unit test * Added example networks to unit test * Fix error in loading disassemblerExample * Network unit test final fixes * Adjusted comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Conditionally require hammer.js to avoid failure on server, on
windowanddocumentreferences.I'm not happy with this solution, but hammerjs/hammer.js#930 suggests there isn't much of a better option for now :/