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

Tracing .lua files #265

Closed
voronin-ivan opened this issue Feb 5, 2022 · 4 comments · Fixed by OptimalBits/bull#2285 or #271
Closed

Tracing .lua files #265

voronin-ivan opened this issue Feb 5, 2022 · 4 comments · Fixed by OptimalBits/bull#2285 or #271

Comments

@voronin-ivan
Copy link

voronin-ivan commented Feb 5, 2022

There are some libraries with .lua files in core, e.g. bull, bullmq, bee-queue. And these files are ignored by default in nft.
I see support for bull in special-cases and I can add the same for other libs. But this way looks difficult to maintain:

  • if the library structure changes, need to add a condition in nft
  • if new libraries appear, need to add them to nft

And then wait for next.js release...

Unfortunately, next.js doesn't have support for "custom" files in API-routes, only in pages (via unstable_includeFiles). And there is no support from Vercel also (includeFiles doesn't work with next.js). So current solutions to work with e.g. bee-queue look uncomfortable:

  1. monkey patch for next.js
  2. or mix /pages/api with /api (and use vercel dev instead of next dev)

Related issues:
#35
taskforcesh/bullmq#469
vercel/next.js#14807

Thank you!

@styfle
Copy link
Member

styfle commented Feb 9, 2022

All assets are traced, regardless of extension, as long as the files are read in a statically analyzable way.

For example, you should be able to do const file = fs.readFile(path.join(__dirname, 'file.lua')).

The best way to solve this problem is to make changes upstream to bull so it doesn't need to be special cased in nft.

I believe changing this line from dir to __dirname should be sufficient https://github.com/OptimalBits/bull/blob/65972184e7dbf156bbf4a4edd8d70a6bf3019b53/lib/commands/index.js#L34

@styfle
Copy link
Member

styfle commented Feb 9, 2022

I created a PR OptimalBits/bull#2285

@voronin-ivan
Copy link
Author

voronin-ivan commented Feb 9, 2022

Thank you for the reply @styfle!

For example, you should be able to do const file = fs.readFile(path.join(__dirname, 'file.lua'))

It doesn't work on the application layer because files from node_modules are ignored.

I do the following in my application (next 12.0.10), just for example:
const file = fs.readFileSync(path.join(__dirname, 'src/addJob.lua')) ok
const file = fs.readFileSync(path.join(__dirname, 'node_modules/bee-queue/lib/lua/addJob.lua')) not ok, file is missing

I believe changing this line from dir to __dirname should be sufficient

Sorry, but this solution doesn't seem to be working. I made the same changes as in your PR, removed special case from nft, and rebuilt the application. After that .lua files are missing.

And your PR won't solve this issue anyway, because there are several libraries with .lua files, not only bull. I mentioned some of them in the first comment.

I can create a repo to help reproduce the problem if needed ;)

@styfle
Copy link
Member

styfle commented Feb 9, 2022

@voronin-ivan Ah yes you're right, since we have the bull special condition.

I had to rename the package to reproduce.

mkdir example
cd example
yarn init -y
yarn add bull
mv node_modules/bull node_modules/bull-example # ensure special case is not hit
echo "require('bull-example')" > index.js
nft print index.js | grep lua

Then I applied my patch and it still didn't work.

It seems like the case with __dirname is not detected.

Once we get that fixed, then the change to bull will work.

@guybedford Would you agree that #267 is a bug?

kodiakhq bot pushed a commit to vercel/next.js that referenced this issue Feb 10, 2022
Related to vercel/nft#265

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
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.

2 participants