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

copy LICENSE to the dist folder #807

Merged
merged 3 commits into from May 16, 2019
Merged

copy LICENSE to the dist folder #807

merged 3 commits into from May 16, 2019

Conversation

zypA13510
Copy link
Contributor

I think in most cases, the dist folder is intended to be used standalone. The lack of a LICENSE file inside the dist folder, however, means anyone who copies the dist folder to use in a project, without copying the accompanying LICENSE file in the parent folder, would violate the MIT license and thus constitute copyright infringement.

This PR makes sure the LICENSE file is copied to the dist folder to allow easier usage.

@alubbe
Copy link
Member

alubbe commented May 9, 2019

I'm not sure the premise is right, because npm install will always install all folders and the current LICENSE file. But the PR itself looks fine - @guyonroche what do you think?

@zypA13510
Copy link
Contributor Author

@alubbe Yes, you are right that npm install installs all folder (because you ran npm publish in the root directory instead of dist, but that's ok).

But the issue mainly comes to using it in the browser environment. In our current setup, we use a grunt script to copy everything we need from node_modules to the build. And in this case, I think it would be reasonable to only copy the dist folder instead of the entire xlsx. And this is how I found the license issue.

@alubbe
Copy link
Member

alubbe commented May 9, 2019

well in that case this solution isn't really good enough because your copy script could also choose to ignore any LICENSE files. I guess the real solution would be the include the content of the LICENSE file into the dist files - there's probably a plugin for that?

@zypA13510
Copy link
Contributor Author

My point is that, the name dist usually implies it can be used alone, while in this case it is not, as it currently stands. And this PR will fix that by adding the missing LICENSE. Now the dist folder can really be used alone (in compliance) without the rest of the files.

However, what the user decides to do with the dist folder is out of our control. For all we know, one can modify every file and remove all license information (e.g. minifiers usually strip out all comments) and there's no stopping that. I think, as long as the dist folder is self-contained, it is the user's responsibility to ensure the proper handling of its content.

@alubbe
Copy link
Member

alubbe commented May 9, 2019

I've never used a dist folder like that, so as per my original comment, I'll defer to @guyonroche whether to merge or not

@guyonroche
Copy link
Collaborator

Personally I'm not over stressed either way. On the one hand I agree with @alubbe in that when used via NPM, the dist folder is hidden behind the contract specified by the package.json file (and documented in the README).

But then, on the other hand, this change is fairly minor and if people do choose to snip out the dist folder for their own purposes I don't have any thing against that. After all as @zypA13510 mentioned - it is functionally complete - so I'm inclined to merge it (after resolving the conflicts of course)

I think though, I might add a word of caution in the README that you're on your own when you do so. The only documented exports are dist/exceljs.js (and exceljs.min.js)

@guyonroche guyonroche merged commit 8c95dba into exceljs:master May 16, 2019
@zypA13510 zypA13510 deleted the patch-1 branch May 16, 2019 07:18
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.

None yet

3 participants