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

chore: change ignore list for node_modules to be less restrictive #4710

Closed
wants to merge 5 commits into from

Conversation

nora-soderlund
Copy link
Contributor

@nora-soderlund nora-soderlund commented Jan 5, 2024

Fixes #3615.

What this PR solves / how to test:
Currently, wrangler ignores any folder that is named node_modules. While intentions are good, this is inherently very restrictive and should not be done. This PR changes the ignore list to only ignore the root folder's node_modules.

Author has addressed the following:

  • Tests
    • Included
    • Not necessary because:
  • Changeset (Changeset guidelines)
    • Included
    • Not necessary because:
  • Associated docs
    • Issue(s)/PR(s):
    • Not necessary because:

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@nora-soderlund nora-soderlund requested review from a team as code owners January 5, 2024 23:30
Copy link

changeset-bot bot commented Jan 5, 2024

🦋 Changeset detected

Latest commit: 57c5c91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

This would be a breaking change for some projects that point to directories that have nested node_modules directories that they don't want to be uploaded.

I think the correct approach is to allow the user to specify their own ignore list via some kind of configuration... perhaps follow the .gitignore or .npmignore files, or perhaps have a special .pagesignore file that does the job.

@nevikashah should chime in as this is a product change.

@nora-soderlund
Copy link
Contributor Author

I've added a .pagesignore alternative.

@GregBrimble
Copy link
Member

Hi, thank you for your PR! Unfortunately, I don't think this is something we're open to implementing right now, but we may revisit this in the future. Thank you for your suggestion.

@GregBrimble GregBrimble closed this Jan 8, 2024
@nora-soderlund
Copy link
Contributor Author

Very unfortunate to see.

@Cherry
Copy link
Contributor

Cherry commented Jan 9, 2024

I'm also quite disappointed to see this closed so hastily for a long-standing bug.

This used to work just fine until ~November 2022 when #2103 was merged. Prior to this, node_modules directory outside of the root were uploaded just fine, but this PR introduced a breaking change and/or regression that hasn't been addressed.

Users have been hitting this since this change: #2240
And it comes up quite regularly in Discord too.

@nora-soderlund
Copy link
Contributor Author

nora-soderlund commented Jan 9, 2024

Can we know of why Cloudflare is not open to implementing this? @GregBrimble / @petebacondarwin

@nevikashah
Copy link
Contributor

nevikashah commented Jan 10, 2024

Hey Nora!

I’m a Product Manager on the Pages/Workers team. I wanted to first off take a moment to thank you for taking the time to write this PR. We are so grateful to have folks in our community like you who are passionate about bettering our products in this workers-sdk repo. We are so lucky to have such an active contributor community - it is such an invaluable source of bug repos, ideas, and indeed, contributions!

In the future, we're going to encourage contributors to first discuss the implementation with the team in the issue before spending time on it in a PR, so we can get alignment on which issues are ready to be worked on, and what the design for that change actually looks like. We're going to add notes to the CONTRIBUTING.md to that effect.

Though this PR wasn't accepted, we want to sincerely thank you for your contribution. We are sorry that we didn't have this process in place before you spent time on the PR and will absolutely do better next time to avoid this situation.

In regards to this PR specifically:

We're in the process of converging the Pages and Workers products which comes with its own set of challenges, and one of the really thorny ones is how to best handle configuration. Workers and Pages have different configuration options today (both because they have different features, and also because they were developed somewhat independently).

They also have different mechanisms for managing that config (Pages is managed almost exclusively in the dashboard where config takes effect with the next deployment and has CLI args for local dev, whereas Workers supports file-based config — wrangler.toml — in-dash config which takes effect immediately and CLI args). We're looking to minimize any surface we can, and adding new config options, regardless of how that is set, would instead just add to the complexity of the problem. We're investigating a long-term and flexible configuration solution, such as adding support for wrangler.toml in Pages. Here, we may allow users to add things like include/exclude rules as part of the static asset config.

So, in the meantime, we have decided to close this PR because rather than add this adhoc fix, we would really like to spend time and think about configuration holistically for Pages/Workers.

Relatedly, the original issue here #3615 is closed as we don't consider that a bug in the current product — we're intentionally not uploading those folders. However, #3176 is a valid feature request, and one we're actively considering in the roadmap as discussed. It might not be a CLI arg, but the general idea of customizing which assets to upload is something that we're actively thinking about.

Again, I want to thank you again for taking the time to put your thoughts and ideas together in this PR. It is absolutely wonderful to see active developers such as yourself contributing to our workers-sdk repo.

@Cherry
Copy link
Contributor

Cherry commented Jan 10, 2024

The insight here is greatly appreciated @nevikashah.

I still don't entirely agree that this is being treated as a Feature Request and not a regression though. This used to work prior to #2103, so while the feature request in #3176 would indeed solve this, it's still technically a regression from prior functional behaviour, and has been reported and raised by both myself and others since 2022.

Nevertheless, thank you for the insight.

@nora-soderlund
Copy link
Contributor Author

nora-soderlund commented Jan 10, 2024

Thank you for the explanation! I agree that for the long term it's more sustainable with a more shared and thought out Pages/Workers approach.

However, for the short term, I would've suggested that an experimental flag is added. Something like --experimental-ignore-list or the alike, just making it explicit that it is a short term solution.

Either way - I understand the decision now that it's been explained, thank you. :)

@robertherber
Copy link

@nevikashah Thanks for a great response explaining the background to this rejection. Still it's very sad to see this not being a higher priority since it's currently breaking all Expo Web projects, requiring manual workarounds specifically for deploying on Cloudflare Pages as reasoned about in #3615 with this current workaround

@nmassey
Copy link

nmassey commented Mar 31, 2024

@nevikashah - thanks also for your great comment above!

I have a couple small (I hope?) requests below ....

Some background

Expo export for web

Like others, I am using Expo export for web (some relevant Expo docs: "Develop websites with Expo" and "Publish websites with Metro"). When exporting for web, Expo (via Metro) bundles web projects into HTML files, JS files, and other required assets (in my case, 7 small PNG image files). Since I am using some standard Expo libraries for navigation, some assets are included via the assets/node_modules/ directory (e.g. for toggle-drawer.png, 'back-icon.png' ... ).

image

Cloudflare Pages with Git Integration

For my project, I set up Cloudflare Pages with Git integration; I loved how easy it was to set up!

But it took me a bit to figure out why my images weren't being shown on my deployed site as expected. Thankfully, I eventually found 🔎 the comments in this PR (and a few other similar issues/PRs). 🤓 (And renaming the folders as suggested in the aforementioned workaround works fine for me. ✅)

A couple of requests 🙏

  1. Would it be possible to update the Cloudflare Pages documentation indicating that all node_modules directories will be ignored? (Or, perhaps this is already in the docs somewhere, and I just missed it?)
  2. On the deployment details screen, for improved developer experience, would it be possible to show a warning message - perhaps something like "8 files were not deployed - click here to see why" ? This could link to some article in the docs indicating, "Cloudflare does not deploy files with such-and-such names, including node_modules ... "

Perhaps even a one-liner log message somewhere around here ... ?

image

Thanks again!

@zhiyongsun
Copy link

cloudflare lost 1 customer for this

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.

🐛 BUG: wrangler: Can not upload assets/node_modules folder to cloudflare pages
8 participants