Skip to content
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

Better checks before using non-standard require methods. Fixes #1723. #1730

Merged
merged 1 commit into from
Mar 13, 2016

Conversation

mathieumg
Copy link
Contributor

What does this PR do?

My stab at fixing #1723.

Where should the reviewer start?

The only modifications are in src/js/base/core/agent.js

How should this be manually tested?

Built the distributable using grunt dist and tested in a Webpack environment. Don't have an immediate access to a Summernote RequireJS build, maybe users like @jumplee can help.

Any background context you want to provide?

To get Webpack to play along, we cannot simply use runtime checks on require like is currently the case. I'm not familiar enough with RequireJS to know if require.specified works at bundle-time or a runtime. The current fix presumes that command is only executed at runtime. (I added a short eval to trick Webpack) It would be great to get feedback from Summernote RequireJS users about this fix, to end the cycle of breaking different bundlers back and forth across releases.

What are the relevant tickets?

if (!hasCodeMirror && isSupportAmd && require) {
if (require.hasOwnProperty('resolve')) {
if (!hasCodeMirror && isSupportAmd && typeof require !== 'undefined') {
if (typeof require.resolve !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be changed with type of require.resolve !== 'undefined' like a 54 line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@hackerwins
Copy link
Member

@mathieumg Thanks for contribution. This commit is works with webpack and requirejs too. Could you fix the line has typeof?

@mathieumg
Copy link
Contributor Author

Done!

@hackerwins
Copy link
Member

@mathieumg Thanks. 👍

hackerwins added a commit that referenced this pull request Mar 13, 2016
Better checks before using non-standard require methods. Fixes #1723.
@hackerwins hackerwins merged commit 3f825c8 into summernote:develop Mar 13, 2016
@mathieumg mathieumg deleted the bugfix_1723 branch March 13, 2016 07:18
@Dorphern
Copy link

Awesome @mathieumg! Clever fix, didn't see that myself :)

@graingert
Copy link
Contributor

@mathieumg This still isn't quite right.

You want:

https://github.com/webpack/docs/wiki/api-in-modules#requireresolveweak

@graingert
Copy link
Contributor

@mathieumg maybe it would be better to have summernote allow CodeMirror to be injected into it?

$('#summernote').summernote({
  height: 300,                 // set editor height
  minHeight: null,             // set minimum height of editor
  maxHeight: null,             // set maximum height of editor
  focus: true                  // set focus to editable area after initializing summernote
  codemirror: require('CodeMirror');
});

This is especially important when everyone switched to ES2015 modules where imports need to be statically resolved.

@mathieumg
Copy link
Contributor Author

@graingert I had stumbled upon resolveWeak, but it says it won't bundle the module whereas we want it to be bundled, if it's available. I wouldn't be against providing codemirror as a parameter, which would only attempt to sniff window.CodeMirror if unspecified. This would allow people with any type of custom bundlers to bundle codemirror (if they so desire) "the right way". What does @hackerwins have to say about it?

ES2015 modules where imports need to be statically resolved

I reckon this is not entirely true, there is System.import which SystemJS and Webpack 2 implement. (Probably others too) See http://exploringjs.com/es6/ch_modules.html#_loader-method-importing-modules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants