search

anseki / plain-draggable

The simple and high performance library to allow HTML/SVG element to be dragged.
https://anseki.github.io/plain-draggable/
MIT License
765 stars 96 forks source link

Import error when using module bundler #2

Closed caghand closed 6 years ago

caghand commented 6 years ago

Hi anseki,

I am a big fan of your libraries, and I would like to report an important issue.

For example, take a look at these two lines in your package.json for plain-draggable:

  "main": "plain-draggable.min.js",
  "jsnext:main": "./src/plain-draggable.js",

This means that when someone imports your package using import PlainDraggable from 'plain-draggable', then he/she will get the minified file plain-draggable.min.js. But, if that person is already using a module bundler (like webpack) that minifies all imported packages, then that bundler will throw an error.

I don't know the exact reason why it does not work, but probably it's because the source is already minified, and it can't be minified again.

In fact, the "main" property in package.json is not meant for the minified version of the library. It is meant for the "jscurrent" version of the library. In the "jsnext" version (./src/plain-draggable.js), you are using ES2015, so the "main" version should simply be the same thing in ES5 (and not minified).

You are probably taking advantage of your existing minification process to convert from ES2015 to ES5. Instead, you can use this page to do the same conversion, but without any minification: http://babeljs.io/repl/

If you want, you can still include the minified file in your package. Whoever needs it can reference that file directly. But what's important is to make sure that the "main" property in package.json points to a file that is not minified.

As a result of this change, webpack users (including most React developers and all users of Create React App) will finally be able to import your package. This should add to the popularity of this excellent package.

This problem is in all the dependencies of plain-draggable as well: anim-event, cssprefix, m-class-list, leader-line, etc. Only after all of them are fixed can the parent package be imported.

Thanks!

-Caghan

anseki commented 6 years ago

Hi @caghand, thank you for the report.

The jsnext:main field means an entry point for ES2015. (Yes, I know that it should be replaced by module.) Therefore, you should specify that (e.g. resolve: {mainFields: 'jsnext:main'}) if you use a bundler that doesn't support the jsnext:main for ES2015.

I'll add module field into package.json at future version. If you want to import current version in a bundler that doesn't support the jsnext:main, please specify that.

anseki commented 6 years ago

Maybe, a reason of the error is that the file that was imported doesn't have export. A source code as jsnext:main has that.

caghand commented 6 years ago

Hi anseki,

Well, I know that the "resolve" parameter in the webpack configuration can work around this. But I did not mention that workaround in my original message, due to these two reasons:

1) Create React App users have no access to the webpack configuration. They can't use this workaround, so you would be excluding a huge group of users. 2) Even if people could use this workaround, it might not be good idea to force people to use jsnext:main for all of their packages, just for the sake of accommodating plain-draggable.

Also, I must point out that there is no guideline that says that developers should use "jsnext:main" for imports (or requires). For CommonJS, UMD, etc., developers have always been using "main" for imports, and it's expected to work. If they want to, developers have the option to use "jsnext:main" for imports. This would be the case if they are using ES2015 modules instead of CommonJS, UMD, etc.

Yes, the "main" script would not have the ES2015 export statement. That is normal. But it would have CommonJS or UMD exports statements. You can see some examples in the npm packages "redux", "react-redux", "tslib", "acorn" and "brcast". This is what makes importing possible. (In our project, we are using hundreds of npm packages, all of whose "main" scripts can be imported.)

Your "main" script seems to have an exports statement, somewhat similar to these examples. But the minimized code is not very clear, so I can't figure out why it's not working. Maybe because multiple packages are being bundled? Or maybe because it is being minified in a certain way? Maybe it's a very minor problem.

By the way, you will see in the examples I have given that the "main" script is not a bundle that contains the package's dependencies. The dependencies are installed as separate packages. It seems that bundling dependencies is optional for "main". Personally, I don't have anything against your "main" script being a bundle that includes dependent packages - I am just trying to figure out a way to get import working with the "main" script.

Thanks!

-Caghan

anseki commented 6 years ago

@caghand, thank you for the advice. As a matter of fact, I am looking for best way. The "main" script contains dependencies it uses and it is built as browser globals (ES5). It doesn't have export. The "jsnext:main" script is source code (ES2015). It has export, and it is importable. Therefore, for the time being, I think that the main field (browser globals) works for users who use the library, and the jsnext:main (i.e. module) field works for developers who import the source code.

Is there a better way to provide for both users (ES5) and developers (ES2015)?

I guess that the module field becomes widespread.

(Sorry, my English is poor.)

caghand commented 6 years ago

Hi @anseki,

No problem! Your English is excellent - I just hope that my writing is also clear. :) Sometimes my sentences are too long, sorry about that.

I think there is confusion around the terms "developer" and "user". As far as I know, the people who consume "main" and the people who consume "jsnext:main" are both users (not developers). People who consume "main" have an old bundler (CommonJS, UMD, etc.), or they are looking for an ES5 version. People who consume "jsnext:main" have a new, modern bundler that supports ES2015 modules. By consuming ES2015 modules, they can take advantage of optimizations like tree shaking. They are not interested in your source code.

Of course, like you said, there are also users who don't want to import your package at all. They just want to include a single file using a <script> tag. So I understand that you want to provide a pre-packaged, ready-to-go, "browser globals" solution. That is fine. But do you really have to reference that minified script from the "main" property? When you do that, you are making it hard for users of old bundlers to import your package.

If you want, you could reference your minified script from the "unpkg" property in package.json. This article explains how "unpkg" works: https://hackernoon.com/how-to-publish-your-package-on-npm-7fc1f5aae600

And then, the "main" property would just reference the unminified version.

By the way, let me mention some references related to minification in npm packages:

Thanks!

-Caghan

caghand commented 6 years ago

Hi @anseki,

I made some more experiments with plain-draggable, and I read a lot of documentation about modules. And I think I have found a good solution that will require very little code change. :)

But first, let me mention this: Like you suspected, the missing export in the "main" script is the cause of the problem. (To be more precise, the missing CommonJS exports statement is the cause.) If I add module.exports = PlainDraggable at the end of the "main" script, importing works perfectly. Previously, I had suspected that the minification step (or maybe the bundling of dependent packages) was the problem. That is not the case.

In your webpack.config.js, if you change libraryTarget to commonjs (instead of var), webpack automatically adds the exports statement at the end of the "main" script. Then, importing works perfectly. However, then there is a different problem: PlainDraggable is no longer available as a browser global, which you prefer to have.

Then I saw this in the webpack documentation (https://webpack.js.org/configuration/output/#module-definition-systems):

libraryTarget: "umd" - This exposes your library under all the module definitions, allowing it to work with CommonJS, AMD and as global variable.

I tried this, and it works perfectly! PlainDraggable is available as a global variable (I was able to consume it using a <script> tag), and webpack import statements work fine!

So with this simple change, you could fix this issue. :)

Thanks!

-Caghan

anseki commented 6 years ago

Hi @caghand, you interact very patiently and kindly. Thank you. :smile:

Of course I know umd option and I used that. However, I replaced that with var because umd made script file become big and that is not required. Also, another big problem about the importing via main is that the packages might not be shared, and those are duplicated, then bundled file becomes too big. As everyone knows, one of the wonderful effectiveness of module system is sharing module. For example, "lodash" is used by many packages and apps. Module-A imports lodash and Module-B, and Module-B imports lodash. That is, lodash is imported by both Module-A and Module-B but only one lodash file should be bundled. If Module-A imports Module-B via main that was already bundled, lodash is imported and bundled twice. Module-A should import source code that is not bundled.

For the time being, the best way to import I think is the importing via jsnext:main/module. I think that the importing via jsnext:main/module is not hard, and also, I guess that the module field becomes widespread.

caghand commented 6 years ago

Hi @anseki,

Hmm, so the script file size increased with the umd option, OK. But it should be negligible. I just checked: The minified file increases from 25,211 bytes to 25,456 bytes only. :) Less than 1%.

When you talk about the problem with "sharing modules," I think you are referring to the fact that "anim-event", "cssprefix" and "m-class-list" are bundled into the "main" script, and so they can't be shared. (Please correct me if I have understood you incorrectly.) I agree that this is a problem. I'd like to make two comments about this problem:

1) It seems to me that these 3 packages are not commonly used in projects in the outside world, other than your own (anseki) projects. So, the people in the outside world who encounter the "module sharing" problem would be very low. 2) In your situation, I would not bundle those 3 packages into the "main" script. I would still provide the bundled file, but as a separate script (that's not assigned to the "main" property). If you think there is a problem with this approach, please tell me.

With my comments, I am trying to find a way to make your package importable by consumers who are following standard module practices. If you really want people to make changes to their webpack configuration so that they can import your package, then you might be losing some potential users. You would be losing all Create React App users since they don't have access to the webpack configuration. :)

It might get better in the future though: I think (but am not 100% sure) that Create React App's default webpack configuration recognizes the module property. So, if you add the module property, I think that Create React App users will be able to import your package. That would be awesome. However, as long as the "main" script is not importable, you would still be excluding users of old bundlers. It's up to you, of course. :)

Thanks!

-Caghan

anseki commented 6 years ago

Thank you for the advice. :smile:

Yes, added file size is small, but that is not required. About the sharing module, when the library uses popular module such as lodash, the problem will become more big. Also, we should support users who use legacy tools that needs bundled file (e.g. Bower, Gulp build script that makes <script> tag via main, and others). And also, I know that main file is not able to be imported. In any case, there is no perfect way now. Unfortunately we can do nothing, or I don't know that... Then, for the time being, the best way to import I think is the importing via jsnext:main/module. At least, both source code that is imported and bundled file that works in ES5 are provided.

caghand commented 6 years ago

I see. So your primary goal is to support legacy build scripts that make a <script> tag via "main". I didn't know that. Now I understand why you prefer to have the bundled & minified file in the "main" property.

In other words, I now understand the role you are giving to the "main" property in general. You are saying that the primary goal of the "main" file is to support making <script> tags. This means that the "main" script should be the bundled & minified file. This kind of file is not a good fit to be used in module-sharing scenarios. Therefore, in general, using the "main" property to import modules is bad practice. And of course, you don't want to activate the umd option for the "main" script, because that would be encouraging this bad practice.

This is clear to me now, and based on your goals, it makes sense for your situation. So, I guess, we can close this issue. :)

As a last comment, I would like to mention that I am eagerly waiting for you to add the module property to plain-draggable, leader-line, and their dependencies. :) If my understanding is correct, this will allow Create React App users to consume your packages without any problems.

Thanks!

-Caghan

anseki commented 6 years ago

Sorry, I couldn't explain well. Yes, I want to support legacy tools too. As far as I know, the legacy tools are still used in many cases, and the legacy tools are especially used in web apps more than Node apps. PlainDraggable is library for web apps. And they have no way to solve when required file is not provided but modern tools have several ways to solve several cases. And also, the script can be imported easily via jsnext:main/module, then umd is not required.

Therefore, for the time being, I think that current package.json is the best way to support both legacy tools and modern tools.

I close this issue. You interacted patiently and kindly. And I learned much by your comment. Thank you! :smile:

anseki commented 6 years ago

And, anyone, please tell me if you know better way to support both legacy tools and modern tools.

caghand commented 6 years ago

Thanks, I learned a lot from your comments, too! Thanks for explaining the situation in detail.

Like I said, as far as I know, among "modern tool users," Create React App users are the only big group who can't use your package, and (I believe) the module property will help with that. :)

davidvink-nofraud commented 5 years ago

"Event listeners help to synchronize with another library. This is a case of LeaderLine."

stated on the PlainDraggable site.

if have both libraries imported

but

lines are not following divs..

Why is that?

anseki commented 5 years ago

Hi @davidvink-nofraud, thank you for the comment. So, this is an issue about the bundling modules. You should not comment here about another issue. Could you make new issue for different topic?

davidvink-nofraud commented 5 years ago

Yes i can. Will do in the morning.

Great package btw!

On Tue, Dec 18, 2018 at 9:32 PM anseki notifications@github.com wrote:

Hi @davidvink-nofraud https://github.com/davidvink-nofraud, thank you for the comment. So, this is an issue about the bundling modules. You should not comment here about another issue. Could you make new issue for different topic?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/anseki/plain-draggable/issues/2#issuecomment-448447650, or mute the thread https://github.com/notifications/unsubscribe-auth/AjZZUUsfgBjTc3BJZKnDkEsueSgpLCkfks5u6aVTgaJpZM4SDBLS .