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

Add option to keep license for certain tree-shaken dependencies #1092

Open
timmb opened this issue Apr 22, 2022 · 10 comments · May be fixed by #1326
Open

Add option to keep license for certain tree-shaken dependencies #1092

timmb opened this issue Apr 22, 2022 · 10 comments · May be fixed by #1326

Comments

@timmb
Copy link

timmb commented Apr 22, 2022

Since #380, the plugin no longer includes licenses for dependencies that have been removed through tree shaking. However, there are occasions where you want to keep them in.

In my case, I am using GLSL code from node modules which is being preprocessed and imported by glslify. The dependency is shaken out but the 3rd party code remains imported within my own GLSL code.

In this situation, it would be helpful to add an option to manually force certain node modules to have their licenses included.

For example,

thirdParty: {
  includePrivate: true,
  output: `${dest}/dependencies.txt`,
  alwaysInclude: [
    'glsl-easings',
  ]     
},
@mjeanroy
Copy link
Owner

mjeanroy commented May 5, 2022

Hi @timmb,

Thanks for the issue!
I'm not sure to understand exactly the problem, could you share a test repo where I can reproduce first?
Thanks!

@timmb
Copy link
Author

timmb commented May 7, 2022

Hi @mjeanroy

Please see here: https://github.com/timmb/mwe-rollup-plugin-license-missing-glsl-licenses

Run npm run build

Code from glsl-noise is bundled into the output via the glslify preprocessor, but this is invisible to rollup-plugin-license and the license is not included in dependencies.txt

I don't anticipate this plugin working with glslify but I currently have no option to make it include the licence for glsl-noise.

See the README in the demo repo for more detail.

@mjeanroy
Copy link
Owner

Hi @timmb,

I'm a bit torn with your proposal: in the example you shared above, glslify is only a build dependency used to compile your own code, it's not used in your final bundle.

Furthermore, glslify should not be a direct dependency of your package, since you don't need it at runtime, it's a devDependency.

I'm not sure if including build dependency in the dependency file output is the right thing to do.
What would be the reason for that?

Btw, a bit out of scope, but #380 is not the cause of your issue, as glslify is not bundled at all.

@timmb
Copy link
Author

timmb commented May 25, 2022

Hi @mjeanroy,

Thank you for looking into this. I should clarify.

It's not the code from glslify that necessitates the license. As you say glslify is a dev dependency. However glslify bundles code from the glsl-noise library. (This happens via the include on line 4 of main.frag, which is GLSL code that is included in sketch.js.)

If you build, open dist.js and search for the string #version 300 es you will see a long string of GLSL code. GLSL is sent to the client which then may pass it to the client's graphics driver to compile. This string is the minified version of the file glsl-noise/simplex/3d.glsl which is from the glsl-noise module.

As code from glsl-noise is in the final distribution, its license also needs to be included.

@mjeanroy
Copy link
Owner

Thanks @timmb for all the details, I definitely agree with your conclusion:

As code from glsl-noise is in the final distribution, its license also needs to be included.

Let me dig a bit deeper, I would like to check if this issue can be fixed without adding a new option.

@nborko
Copy link

nborko commented May 31, 2022

I came here looking for the same feature, except I'm bundling 3rd party CSS: in the case of modern-normalize, in it's entirety; in the case of TailwindCSS, a subset based on usage, but licensed code (MIT) nonetheless.

@mjeanroy
Copy link
Owner

mjeanroy commented Jun 1, 2022

Hi @timmb,

The main problem here is that the glslify rollup plugin transform the code, and rollup has no way to know which modules have been loaded or not (another side effect is that files are not watchable by rollup as well).

I updated the plugin implementation to make rollup aware of these files, you can see here.

You can review the diff commit by commit, the last commit is where the "magic" happens.

Basically, what I did:

  • Use depper.on to be aware of loaded files.
  • Tweak a bit the output code to import these files (and replace it by a single line of comment).

Then, the file is correctly loaded, and everything works fine.

I don't know if it could be simplified, I would love to see a way to force rollup to include modules in the chunk modules, that could make things way easier.

@mjeanroy
Copy link
Owner

mjeanroy commented Jun 1, 2022

@nborko could you share a test repo where I can reproduce?
That would help me to understand if it is the same issue or not 🙂

@nborko
Copy link

nborko commented Jun 9, 2022

As I think about it, it's not really the same issue. The issue with tailwind is that it gets generated via a postcss plugin, so it's not being directly imported in a way that rollup can see it.

But it would be nice if the license plugin had an option to add package names of packages that exist in node_modules as a dependency that get built into a bundled product, even if rollup doesn't itself see it during the bundling process.

@gwatts gwatts linked a pull request Jan 13, 2023 that will close this issue
@mitar
Copy link

mitar commented Feb 29, 2024

Off topic: For TailwindCSS, I must say that I think (but I am not a lawyer) that one does not have to include a banner for it. You are not bundling their code, just the output of the program. See my comments here and here, if you want to engage into discussion about that. Creator of TailwindCSS also claims there is no need to include a copyright notice.

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 a pull request may close this issue.

4 participants