fix: Update fabric.js to v3.3.2 & modify webpack externals configuration (fix #129, #134, #219) #234

junghwan-park · 2019-08-02T02:40:59Z

Please check if the PR fulfills these requirements

It's submitted to right branch according to our branching model
It's right issue type on title
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
It does not introduce a breaking change or has description for the breaking change

Description

[email protected] update (update: Update Fabric.js version #129)
Add fabric.js to webpack.externals in karma.conf.json for karma test (Fabric is undefined in test environment #134)
Modify externals.fabric config in webpack.config.js ( Module '"tui-image-editor"' has no default export. #219)

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

…xternal config (fix #129, #219)

…134)

jinwoo-kim-nhn · 2019-08-02T07:14:19Z

karma.conf.js

@@ -121,6 +122,9 @@ module.exports = function(config) {
 reporters: ['dots'],
 webpack: {
 devtool: 'inline-source-map',
+ externals: {
+ fabric: 'fabric'
+ },


externals와 files에 꼭 추가 해줘야 동작하나요? 제 생각엔 node_modules/밑에 이미 설치 되어 있기때문에 굳이 external로 추가 안해도 동작할것 같아서요

결과부터 말씀드리면, 실제 web에서 사용할 때와 같이 external처리 하지 않으면 karma에서 fabric을 제대로 가져오지 못해 테스트가 동작하지 않습니다.

우선 이렇게 처리한 원인에 대해 설명을 해볼게요. fabric을 external 처리 해주지 않으면 webapck이 같이 번들링을 해버리는데, 그렇게 되면 fabric을 CommonJS로 사용하게 됩니다.
근데, 이때 문제가 생기는게 fabric이 CommonJS로는 module.exports = fabric가 아닌module.exports.fabric = fabric와 같이 내보내집니다. 실제 fabric 모듈이 한 depth더 들어가 있는 형태에요.
하지만, 웹에서 사용할때는 fabric 모듈이 window.fabric으로 정의되어 있죠. 그냥 그래서 fabric으로 사용해야해요.

그래서 같은 코드로는 도저히 실 사용환경, 테스트 환경 모두에서 잘 동작하지 않습니다. karma를 위해 named import를 하면 실제 애플리케이션이 동작하지 않고, 반대로 웹 환경에서 fabric을 가져오듯 1-depth 객체를 사용하면 karma 테스트에서 제대로 읽어들이지 못하는 거죠.

근데 테스트 환경이 이렇게 구성된게 잘못되었다는 생각이 들었어요. 이미지에디터는 본래 웹 브라우저에서 동작하게 설계되어 있으므로, karma test에서도 웹 상황과 동일하게 external 처리한 fabric을 브라우저에 따로 로드해서 가져와야 할 것 같았습니다. 그렇게 적용한 결과, 같은 코드로 karma 테스트할때도 잘 동작했습니다.

그래서 이렇게 externals 처리하고 따로 files로 불러오도록 수정된 것입니다. :)

아;; 이제야 이해되네요. 넵 설명 감사해요. 수고하셨습니다.

dongsik-yoo · 2019-08-02T07:17:54Z

karma.conf.js

@@ -121,6 +122,9 @@ module.exports = function(config) {
 reporters: ['dots'],
 webpack: {
 devtool: 'inline-source-map',
+ externals: {
+ fabric: 'fabric'


~~webpack.config.js 파일에서는 "fabric.fabric": "fabric" 예요. 달라도 되는지 확인 부탁 드립니다.~~
아, 수정했네요.

jinwoo-kim-nhn · 2019-08-02T07:20:38Z

webpack.config.js

@@ -42,6 +41,12 @@ module.exports = {
 'commonjs2': 'tui-color-picker',
 'amd': 'tui-color-picker',
 'root': ['tui', 'colorPicker']
+ },
+ 'fabric': {
+ 'commonjs': ['fabric', 'fabric'],


commonjs 로는 var fabric = require('fabric').fabric 요로케 불러와야 되는군요 ;; ㅎㅎ

네 맞아요... ㅠㅠ 모듈 방식에 따라 fabric이 정의된 depth가 달라요.

jinwoo-kim-nhn · 2019-08-02T07:22:05Z

리뷰 완료했습니다. 수고하셨어요

dongsik-yoo · 2019-08-02T07:29:56Z

수고하셨습니다.

…ion (fix nhn#129, nhn#134, nhn#219) (nhn#234) * fix: Resolve jQuery vulnerability * fix: Change fabric import method named to default & correct webpack external config (fix nhn#129, nhn#219) * fix: Add fabric.js to external library into karma webpack config (fix nhn#134) * fix: Add half stroke width to center added icon's left, top

* revert #234

* env: remove dependencies related `karma` * remove unused dependency `simulant` * chore: remove directory from `.gitignore` * chore: remove karma from `bower.json` * chore: remove `jasmine`, `jquery` from `.eslintrc.js` * env: add `jest` * env: update dependencies related `eslint` * chore: remove `eslint-loader` in `webpack.config.js` * chore: remove `karma.config.js` * env: add `eslint-plugin-jest` * chore: add `eslint-plugin-jest` config to `.eslintrc.js` * chore: update test command * env: add jest config * add `jest-esm-transformer` dependency * env: add `jest-canvas-mock` * fix: resolve webpack alias * refactor: remove globals in `.eslintrc.js` * refactor: remove `fabric` in webpack `externals` * revert #234 * refactor: apply jest to `cropper.spec.js` * env: add `jest-svg-transformer` * refactor: add `dot-notation` rule to `.eslintrc.js` * change `max-nested-callbacks` rule's max option to 5 * refactor: apply jest to `action.spec.js` * add `no-undefined` rule * refactor: apply jest to `arrowLine.spec.js` * env: add file mocking to jest * refactor: apply jest to `command.spec.js` * refactor: apply code review * apply `toMatchObject` * apply `toMatchSnapshot` * add `jest/expect-expect` rules to `.eslintrc.js` * refactor: apply code review 2 * apply `async`, `await` instead `return Promise` * add `parserOptions.ecmaVersion` to `.eslintrc.js`

* env: remove dependencies related `karma` * remove unused dependency `simulant` * chore: remove directory from `.gitignore` * chore: remove karma from `bower.json` * chore: remove `jasmine`, `jquery` from `.eslintrc.js` * env: add `jest` * env: update dependencies related `eslint` * chore: remove `eslint-loader` in `webpack.config.js` * chore: remove `karma.config.js` * env: add `eslint-plugin-jest` * chore: add `eslint-plugin-jest` config to `.eslintrc.js` * chore: update test command * env: add jest config * add `jest-esm-transformer` dependency * env: add `jest-canvas-mock` * fix: resolve webpack alias * refactor: remove globals in `.eslintrc.js` * refactor: remove `fabric` in webpack `externals` * revert #234 * refactor: apply jest to `cropper.spec.js` * env: add `jest-svg-transformer` * refactor: add `dot-notation` rule to `.eslintrc.js` * change `max-nested-callbacks` rule's max option to 5 * refactor: apply jest to `action.spec.js` * add `no-undefined` rule * refactor: apply jest to `arrowLine.spec.js` * env: add file mocking to jest * refactor: apply jest to `command.spec.js` * refactor: apply code review * apply `toMatchObject` * apply `toMatchSnapshot` * add `jest/expect-expect` rules to `.eslintrc.js` * refactor: apply code review 2 * apply `async`, `await` instead `return Promise` * add `parserOptions.ecmaVersion` to `.eslintrc.js` * refactor: apply jest to `cropzone.spec.js` * refactor: apply jest to `drawingMode.spec.js` * refactor: apply jest to `filter.spec.js` * refactor: apply `async`, `await` instead `return Promise` * refactor: apply jest to `flip.spec.js` * refactor: apply jest to `graphics.spec.js` * refactor: apply jest to `history.spec.js` * refactor: apply jest to `icon.spec.js` * refactor: apply jest to `imageEditor.spec.js` * refactor: remove unused env * refactor: change `isFunction` to use `typeof` * apply to `invoker.js` * refactor: apply jest to `invoker.spec.js` * refactor: apply jest to `line.spec.js` * refactor: apply jest to `promiseApi.spec.js` * refactor: apply jest to `resize.spec.js` * refactor: apply jest to `rotation.spec.js` * refactor: apply jest to `selectionModifyHelper.spec.js` * refactor: apply jest to `shape.spec.js` * refactor: apply jest to `text.spec.js` * refactor: apply jest to `theme.spec.js` * refactor: apply jest to `ui.spec.js` * refactor: apply jest to `uiRange.spec.js` * refactor: apply jest to `zoom.spec.js` * refactor: change to `toMatchObject` from `expect.objectContaining` * chore: apply code review

junghwan-park added 4 commits July 15, 2019 14:46

fix: Resolve jQuery vulnerability

2c03da4

fix: Change fabric import method named to default & correct webpack e…

d90e2eb

…xternal config (fix #129, #219)

fix: Add fabric.js to external library into karma webpack config (fix #…

2cb0c70

…134)

fix: Add half stroke width to center added icon's left, top

d68b824

junghwan-park requested a review from dongsik-yoo August 2, 2019 02:41

jinwoo-kim-nhn reviewed Aug 2, 2019

View reviewed changes

dongsik-yoo reviewed Aug 2, 2019

View reviewed changes

jinwoo-kim-nhn reviewed Aug 2, 2019

View reviewed changes

junghwan-park merged commit a50444c into master Aug 2, 2019

junghwan-park deleted the fix/fabric branch August 2, 2019 08:24

lja1018 added a commit that referenced this pull request Aug 25, 2021

refactor: remove fabric in webpack externals

185f8e4

* revert #234

lja1018 mentioned this pull request Aug 25, 2021

env: remove karma, add jest 1 #628

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Update fabric.js to v3.3.2 & modify webpack externals configuration (fix #129, #134, #219) #234

fix: Update fabric.js to v3.3.2 & modify webpack externals configuration (fix #129, #134, #219) #234

junghwan-park commented Aug 2, 2019

jinwoo-kim-nhn Aug 2, 2019 •

edited

Loading

junghwan-park Aug 2, 2019

jinwoo-kim-nhn Aug 2, 2019 •

edited

Loading

dongsik-yoo Aug 2, 2019 •

edited

Loading

jinwoo-kim-nhn Aug 2, 2019

junghwan-park Aug 2, 2019

jinwoo-kim-nhn commented Aug 2, 2019

dongsik-yoo commented Aug 2, 2019

fix: Update fabric.js to v3.3.2 & modify webpack externals configuration (fix #129, #134, #219) #234

fix: Update fabric.js to v3.3.2 & modify webpack externals configuration (fix #129, #134, #219) #234

Conversation

junghwan-park commented Aug 2, 2019

Please check if the PR fulfills these requirements

Description

jinwoo-kim-nhn Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

junghwan-park Aug 2, 2019

Choose a reason for hiding this comment

jinwoo-kim-nhn Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

dongsik-yoo Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

jinwoo-kim-nhn Aug 2, 2019

Choose a reason for hiding this comment

junghwan-park Aug 2, 2019

Choose a reason for hiding this comment

jinwoo-kim-nhn commented Aug 2, 2019

dongsik-yoo commented Aug 2, 2019

jinwoo-kim-nhn Aug 2, 2019 •

edited

Loading

jinwoo-kim-nhn Aug 2, 2019 •

edited

Loading

dongsik-yoo Aug 2, 2019 •

edited

Loading