naxodev / oss

Open Source Software (OSS) projects by naxodev
https://naxo.dev
MIT License
19 stars 1 forks source link

[nx-cloudflare] `./bin` is not exported from @cloudflare/next-on-pages #30

Closed khalil-omer closed 10 months ago

khalil-omer commented 10 months ago

Current Behavior

Thanks for creating nx-cloudflare!

Please see this issue I raised in Cloudflare's next-on-pages repo. A CF maintainer has responded. They prefer this issue be addressed in your repo by changing the way next-on-pages is resolved when it is called from nx-cloudflare.

Proposed change from CF team member to this file:

-      require.resolve('@cloudflare/next-on-pages/bin'),
+      path.resolve(require.resolve('@cloudflare/next-on-pages/next-dev'), '../../..'),

Expected Behavior

Please see the linked issue.

GitHub Repo

No response

Steps to Reproduce

  1. Set up a project with @naxodev/nx-cloudflare (or manually try to reference node_modules/@cloudflare/next-on-pages/.bin/index.js
  2. Run a build command programmatically
  3. Observe the error related to the package subpath './bin'.

Nx Report

This issue is not specific to Nx. It's an issue of Node behavior when running require() against another module that does not export the required path.

Failure Logs

This is working fine for me now because I add to add postinstall script to patch @cloudflare/next-on-pages.

Operating System

Additional Information

No response

NachoVazquez commented 10 months ago

Hey, @khalil-omer, thanks for the detailed issue!

I'm trying to understand if the @cloudflare/next-on-pages/bin was removed in some recent version of @cloudflare/next-on-pages.

As you can see in the repo, we have some tests checking that the plugin is working as expected. Could you shed some light on this question?

Beyond that, I'm happy to change how we access the @cloudflare/next-on-pages API as long as the current tests pass. I'm facing some issues with the newest version of Nx, but once those get fixed, I will update all packages and release a new version. I will include this fix in there.

dario-piotrowicz commented 10 months ago

Hi @NachoVazquez šŸ‘‹

I don't think that the bin was ever getting exported, as you can see it wasn't even exposed in the initial commit: https://github.com/cloudflare/next-on-pages/blob/2864175417fa068f908bc9228e15a0f2b1201382/package.json

And I don't think the export was added and then removed, so if you are indeed testing your integration with it I am not really sure how it could have worked before šŸ˜• šŸ¤”

Could something on your end have changed?

Anyways regarding exporting the bin, as I mentioned in the next-on-pages repository I am happy to do it if there is no other option, but since the package is only supposed to be used as a CLI I would prefer not to export it (as it feels semantically wrong to me šŸ˜…) so if you could rely on the next-dev submodule as mentioned in the issue that'd be great šŸ˜ƒ (PS: I also just remembered that we prefixed that with __experimental__ for the time being šŸ˜“, if you're willing to use it, I will try to remove the prefix as soon as possible so you can use the proper next-dev entry šŸ™)

NachoVazquez commented 10 months ago

Hi @dario-piotrowicz ! Thanks for the details. I have to look deeper at this. I am trying to understand why it works on my example project but not others.

The proposed solution is not working on the example project (Error: Cannot find module '@cloudflare/next-on-pages/next-dev') but probably because of my setup.

I'll take a closer look and return to you, but this is something I can fix.

Thank you and @khalil-omer for bringing this issue to my attention.

dario-piotrowicz commented 10 months ago

@NachoVazquez as I said probably right now it'd probably need to be @cloudflare/next-on-pages/__experimental__next-dev šŸ„²

NachoVazquez commented 10 months ago

@dario-piotrowicz Oh, that's true. Anyway, I need to prepare my e2e tests in a way that catches the current problem. Thanks again.

NachoVazquez commented 10 months ago

Hi @dario-piotrowicz, I'm back working on this issue. I could reproduce the issue in an e2e test and try the proposed solution. The build process runs but doesn't create any of the build artifacts. Beyond that, it isn't the right thing to do since that module is intended for the development server, which, in theory, wouldn't create an optimized bundle.

I appreciate your thoughts on this, but in the meantime, I propose to apply a similar trick to the one @khalil-omer is using and patch myself your package.json to export the bin/index.js module and build the project as I was doing before. That way, you don't have to export it yourselves if you don't want to.

khalil-omer commented 10 months ago
@NachoVazquez here's my script ```js const fs = require('fs'); const path = require('path'); const packagePath = path.join( __dirname, '..', 'node_modules', '@cloudflare', 'next-on-pages', 'package.json' ); try { const packageJson = require(packagePath); packageJson.exports = { ...packageJson.exports, './bin': './bin/index.js', }; fs.writeFileSync(packagePath, JSON.stringify(packageJson, null, 2)); console.log( '@cloudflare/next-on-pages package.json has been patched successfully.' ); } catch (error) { console.error( 'Error patching @cloudflare/next-on-pages package.json:', error ); process.exit(1); } ```
NachoVazquez commented 10 months ago

Thanks @khalil-omer

NachoVazquez commented 10 months ago

After many attempts, I returned to the original implementation and used npx to run the command.

This aligns with the intention of the next-on-pages team.

I will release the fix today.

NachoVazquez commented 10 months ago

@khalil-omer, the tests are passing, but please let me know if you have any issues with the new version 1.0.1.

dario-piotrowicz commented 10 months ago

Thanks a bunch @NachoVazquez :smiley:

Yeah using npx sounds like the most appropriate solution to me (but again if there were no other/better alternatives I was willing to add the export in our package.json although using npx does much more appropriate to me :smiley:)

Fingers crossed that it all works fine now :slightly_smiling_face:

NachoVazquez commented 10 months ago

I'll keep you posted @dario-piotrowic. Thanks for your assistance with this.