Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

Add support for assets not mentioned in files of package.json #77

Closed
timsuchanek opened this issue Dec 10, 2019 · 6 comments
Closed

Add support for assets not mentioned in files of package.json #77

timsuchanek opened this issue Dec 10, 2019 · 6 comments

Comments

@timsuchanek
Copy link
Contributor

While zip-it-and-ship-it right now supports resolving require statements, if I have a path.join(__dirname, 'somefile') statement, it gets ignored by zip-it-and-ship-it, therefore if a package ships some assets, which are not mentioned in files of the package.json, they're not included by the packing algorithm.

With Photon.js, we're downloading binary files in the postinstall hook of the package. These files right now are ignored by zip-it-and-ship-it, making Photon.js broken in Netlify functions.

Note: The binaries are not mentioned in the files field of our package.json, as we don't want to ship the binaries when publishing. Instead, they're downloaded in the postinstall hook of the @prisma/photon package.

I see a couple of options here:

  1. Give package authors an explicit way to add files in addition to files.
    This option would, however, require a custom standard, library authors shouldn't really have to bother about concrete deployment targets like Netlify.

  2. Just copy over the whole packages, not just what's mentioned in the files part.

  3. Use smart logic like node-file-trace does (it's used in Zeit Now).
    Photon.js is compatible with node-file-trace, by adding path.join(path to binary) statements in the code, which are recognized by node-file-trace.

The first step for this smart logic could basically be, including not only the require statements of a file, but also the path.join / path.resolve statements.

A quick fix could, of course, be having a union of what you have today and the files that node-file-trace suggests.

I know that you're having a beta implementation, which may have tackled this already, but I don't have access to the build beta yet. I just filled the beta form, in case that's needed.

Thanks!

@timsuchanek
Copy link
Contributor Author

Update: It looks like https://github.com/netlify/zip-it-and-ship-it/pull/48/files#diff-6ff379484cbabad48301d485db111c08R48 should solve this. Now I just need to know how to try the beta 🙂

@ehmicky
Copy link
Contributor

ehmicky commented Dec 10, 2019

Hi @timsuchanek, thanks for reaching out!

For reference purpose, I believe you are speaking about the following postinstall script which triggers the following file. Could you please print a tree of files that are typically generated by this Node file?

Please note also that the problem you are mentioning are path.join() is a broader issue: we do not currently support dynamic require(), for example when the path is not a string constant (which is your case) or when put inside a if block. I've opened #68 for this specific problem, feel free to provide with some feedback on this.

As you correctly pointed out, the beta should include a fix for your particular issue. I am going to make sure you're included in our beta so you can make sure Photon.js and Netlify Functions work well together 👍

I will bump this issue once you have access :)

@timsuchanek
Copy link
Contributor Author

timsuchanek commented Dec 10, 2019

For reference purpose, I believe you are speaking about the following postinstall script which triggers the following file .

100% Correct!

Could you please print a tree of files that are typically generated by this Node file?

Yes, the files that are already shipped with the @prisma/photon package and are correctly packaged already by Netlify are these:
https://photonjs-324-test.netlify.com/.netlify/functions/hello-facade

The files, which are generated on top in node_modules/@prisma/photon:
index.js
index.d.ts
runtime/query-engine-PLATFORM Where PLATFORM can e.g. be darwin or in netlifys case rhel-openssl-1.0.x.

@ehmicky
Copy link
Contributor

ehmicky commented Dec 10, 2019

Yes I can confirm should be packaged with the Netlify Build Beta. I will let you know once you're in! :)

@timsuchanek
Copy link
Contributor Author

I just tried it out with the build beta - it works!

@ehmicky
Copy link
Contributor

ehmicky commented Dec 11, 2019

Amazing Tim! Feel free to send us more feedback about the Beta on netlify/zip-it-and-ship-it (for Netlify Functions) and on netlify/build (for the rest). 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants