-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: Update fabric.js to v3.3.2 & modify webpack externals configuration (fix #129, #134, #219) #234
Conversation
@@ -121,6 +122,9 @@ module.exports = function(config) { | |||
reporters: ['dots'], | |||
webpack: { | |||
devtool: 'inline-source-map', | |||
externals: { | |||
fabric: 'fabric' | |||
}, |
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.
externals와 files에 꼭 추가 해줘야 동작하나요? 제 생각엔 node_modules/밑에 이미 설치 되어 있기때문에 굳이 external로 추가 안해도 동작할것 같아서요
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.
결과부터 말씀드리면, 실제 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로 불러오도록 수정된 것입니다. :)
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.
아;; 이제야 이해되네요. 넵 설명 감사해요. 수고하셨습니다.
@@ -121,6 +122,9 @@ module.exports = function(config) { | |||
reporters: ['dots'], | |||
webpack: { | |||
devtool: 'inline-source-map', | |||
externals: { | |||
fabric: 'fabric' |
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.
webpack.config.js 파일에서는 "fabric.fabric": "fabric" 예요. 달라도 되는지 확인 부탁 드립니다.
아, 수정했네요.
@@ -42,6 +41,12 @@ module.exports = { | |||
'commonjs2': 'tui-color-picker', | |||
'amd': 'tui-color-picker', | |||
'root': ['tui', 'colorPicker'] | |||
}, | |||
'fabric': { | |||
'commonjs': ['fabric', 'fabric'], |
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.
commonjs 로는 var fabric = require('fabric').fabric
요로케 불러와야 되는군요 ;; ㅎㅎ
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.
네 맞아요... ㅠㅠ 모듈 방식에 따라 fabric
이 정의된 depth가 달라요.
리뷰 완료했습니다. 수고하셨어요 |
수고하셨습니다. |
…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
* 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
Please check if the PR fulfills these requirements
fix #xxx[,#xxx]
, where "xxx" is the issue number)Description
[email protected]
update (update: Update Fabric.js version #129)webpack.externals
inkarma.conf.json
for karma test (Fabric is undefined in test environment #134)externals.fabric
config inwebpack.config.js
( Module '"tui-image-editor"' has no default export. #219)Thank you for your contribution to TOAST UI product. 🎉 😘 ✨