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

Removed test folder from the package #59

Closed
wants to merge 2 commits into from

Conversation

mikhail-g
Copy link

@mikhail-g mikhail-g commented Nov 21, 2023

There was a strange issue when building with bazel the lib wdio-vscode-service which has a dependency on @fastify/send. It breaks on the path with non-ASCII char U+2603 ☃ node_modules/@fastify/send/test/fixtures/snow ☃/index.html

A workaround for this: rm -rf node_modules/@fastify/send/test in postinstall script

Checklist

@mikhail-g
Copy link
Author

To test the change

  1. run npm pack
  2. unzip fastify-send-2.1.0.tgz
  3. open package folder
    Result: see that test folder is not included in the package

There was a strange issue when building with bazel
the lib wdio-vscode-service which has a dependency on
@fastify/send. It breaks on the path with non-ASCII char U+2603 ☃
node_modules/@fastify/send/test/fixtures/snow ☃/index.html
Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

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

@mcollina
Copy link
Member

I think a better way to fix this is to have that directory dynamically created in the tmp folder.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 22, 2023

Maybe it is better to report this upstream as this could be a source of error anyway.

@mikhail-g
Copy link
Author

Please see fastify/skeleton#42 (comment)

I do not see copy-paste from .gitignore to .npmignore as a big support issue. I can create an automatic script for that and put it in "prepack"

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

I think this should be addressed upstream first.

@mikhail-g
Copy link
Author

I think this should be addressed upstream first.

@Uzlopak, do you mean at https://github.com/fastify/fastify/blob/main/.npmignore ?

@mikhail-g
Copy link
Author

The test failure is not relevant to the change at CI / test / Test (21, ubuntu-latest

Run actions/setup-node@v4
  with:
    node-version: [2](https://github.com/fastify/send/actions/runs/6959139610/job/18936272961?pr=59#step:3:2)1
    always-auth: false
    check-latest: false
    token: ***
Attempting to download 21...
Not found in manifest. Falling back to download directly from Node
Error: Unable to find Node version '21' for platform linux and architecture x6[4](https://github.com/fastify/send/actions/runs/6959139610/job/18936272961?pr=59#step:3:4).

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 22, 2023

No. This is an issue with bazel and wdio-vscode-service. So report this issue to bazel.

@kibertoad
Copy link
Member

I'd say that it needs to be addressed in both places. It needs to be reported to Bazel, but we also shouldn't be breaking our users either.

@mikhail-g
Copy link
Author

@Uzlopak I got your point. I also contacted bazel before.

But my point is:

  1. Having folders with whitespaces is the name is unusual practice and some other libs/frameworks could fail while working with them. So it will limit the application of this lib in those cases without giving any additional value
  2. Adding send as a dependency I never intend to run it's tests or use them in any other way. So removing the folder will make this lib lighter, which is preferable

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 22, 2023

anyway.

this PR should create the folder when testing and not ignore it in npmignore as @mcollina mentioned.

@mikhail-g
Copy link
Author

Is there any reason why this lib should have test folder in the package?

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 23, 2023

Yes, we like to ship them with the npm packages.

@mikhail-g
Copy link
Author

Yes, we like to ship them with the npm packages.

but why? is there any purpose for that?

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 23, 2023

You can read all about it here

@mikhail-g mikhail-g mentioned this pull request Nov 23, 2023
2 tasks
@Fdawgs Fdawgs closed this May 16, 2024
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.

5 participants