fix(rollup, webpack): print license in bundled file #2487
Conversation
There was a problem hiding this comment.
The output matches expectations, so that's good!
Do we care about preservation of the vtk.js license in downstream app bundles? If so, we will need to start the license block with a /*! . Additionally, one concern I have with the ESM build is the potential for the license to be replicated many times, once per unique vtk.js import. This will have to be tested to be sure.
rollup.config.js
Outdated
| intro() { | ||
| return `/* ${fs.readFileSync( | ||
| path.resolve(__dirname, './LICENSE'), | ||
| 'utf8' | ||
| )} */`; | ||
| }, |
There was a problem hiding this comment.
Is intro() called once overall, or once per file? If the latter, it's best to not have the overhead of reading the LICENSE file multiple times. If just once overall, then this is fine.
There was a problem hiding this comment.
It is run for the output for which it is in the output options so here for all outputs in outputdir. I'll make an update for that.
There was a problem hiding this comment.
The license is printed only in vtk.js file now. However, I did not get to do it without creating another entry/output "rule" for that.
There was a problem hiding this comment.
FYI the vtk.js file isn't the ESM build's entrypoint. I think it's the index.js file. That being said, the import strategy we have doesn't go through a single file, so we would need to consider how we want the vtk.js license to show up in downstream builds.
@finetjul would we consider a single license header in the vtk.js file to be sufficient? Or in each outputted .js file? We would still have the issue of preserving the license header if we output to each .js file, but we may not have any such license header if the user doesn't include vtk.js.
There was a problem hiding this comment.
@finetjul so currently the license is printed in the vtk.js file but as said by Forrest, if the user does not include the vtk.js file it will not be written.
joannacirillo
force-pushed
the
print-licence-build
branch
3 times, most recently
from
ee740ba
to
7b3df29
Compare
rollup.config.js
Outdated
| format: 'es', | ||
| hoistTransitiveImports: false, | ||
| intro() { | ||
| return `/* ${fs.readFileSync( |
There was a problem hiding this comment.
Do we want to preserve the license in downstream apps that build using vtk.js? This is typically accomplished via /*! or a line with @license.
There was a problem hiding this comment.
Needing to preserve will complicate how we get it to be preserved...
joannacirillo
force-pushed
the
print-licence-build
branch
2 times, most recently
from
dd0ad9a
to
d61afc6
Compare
joannacirillo
force-pushed
the
print-licence-build
branch
from
d61afc6
to
6654456
Compare
|
Lot's of |
My bad, it should be build commits, only one commit in the end even. |
finetjul
force-pushed
the
print-licence-build
branch
from
6654456
to
b8f6e91
Compare
Results
Configuration of Webpack and Rollup have been updated to write license file content in bundles (in prod and dev builds) for esm and umd builds.
npm run reformatto have correctly formatted code