Skip to content

Conversation

banderson
Copy link
Contributor

@banderson banderson commented Aug 13, 2024

Edit (2024-08-16):

After talking this over with the team and experiencing additional support issues related to root-level package.json, we all think it's probably best to remove the whole file at this point (25d1e4a). So the details below are still accurate, but some the details don't matter as much.

I was able to scaffold a new private app without generating package.json files, and upload the newly generated project, and it all worked end-to-end:

Screenshot 2024-08-16 at 12 37 42 PM

Aside: I was surprised to see the warning in the CLI about missing package.lock files. I'm not touching that here, but maybe someone else can take a look at that piece? ¯\_(ツ)_/¯


While testing out a new project this past week, and trying to emulate some of the patterns I've seen in customer extensions (namely, devs using yarn or pnpm package managers over npm), I found a couple issues:

  • postinstall script assumes npm as package manager, which means it will automatically generate package-lock.json files in several directories, leading to warnings from other package manager CLIs and tooling.
  • postinstall script isn't resilient to project structure changes. After removing the app.functions directory (I have no serverless functions), that led to the CLI hanging for a long time, and eventually giving me an obscure error message when it failed. I don't have exact repro steps for the error message I saw the first time, but it still hangs for about a minute:

Kapture 2024-08-13 at 19 01 05

These scripts served their purpose as a crutch to get around friction points (having to jump around to different directories to run commands & manage packages) but at this point it seems less helpful and would be a net negative if customers hit the edge cases. Perhaps we're covering up issues by including them here, as well.

Minor changes:
I also added private: true to make sure it doesn't get accidentally published, and removed the name field, as this has become just one more thing for me to remember to change or delete after scaffolding a project. It should not be required unless publishing to public repository.

While testing out a new project this past week, and trying to emulate some of the patterns I've seen in customer extensions, I found a couple issues:

 - `post-install` script assumes npm as package manager, which means it will automatically generate package-lock.json files in two directories
 - `post-install` and `build` scripts aren't resilient when project structure changes. After removing the `app.functions` directory (I have no serverless functions), that led to the CLI hanging and giving me an obscure error message when it failed

These scripts served their purpose as a crutch to get around friction points (having to jump around to different directories to run commands & manage packages) but at this point it seems less helpful and would be a net negative if customers hit the edge cases. Perhaps we're covering up issues by including them here, as well.
Copy link
Contributor

@brandenrodgers brandenrodgers left a comment

Choose a reason for hiding this comment

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

That all sounds reasonable to me 👌

@brandenrodgers
Copy link
Contributor

Should we apply these changes to both the public and private apps?

@@ -1,11 +1,7 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would it be beneficial to remove this file entirely? Running npm install at the wrong directory level seemed to be a pretty common support issue, and the post install was mitigating this in at least some of the cases. Are there benefits to keeping this file around without the scripts block?

Copy link
Contributor Author

@banderson banderson Aug 15, 2024

Choose a reason for hiding this comment

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

I could not agree more, @joe-yeager! I took a baby step to remove the fangs from this issue with this small change, but I am also happy if we go all the way and remove it!

@banderson
Copy link
Contributor Author

@joe-yeager @brandenrodgers I totally forgot that npm will still handle npm install commands in the project root, and for me it creates a new package.json file in my user root directory 😮. Welp, we can't fight that battle. This is still an improvement and should help avoid confusion, even if it can't fix everything.

Kapture 2024-08-16 at 12 23 29

@banderson
Copy link
Contributor Author

This is the test flow, and this change works, as in no package.json's anymore. You can see the surprise moment at the end where npm i in project root still worked, but I covered that part specifically in the comment above ☝️
Kapture 2024-08-16 at 12 22 07

@banderson banderson changed the title chore: remove potentially unreliable package.json scripts chore: remove package.json from private app project Aug 16, 2024
Copy link
Contributor

@brandenrodgers brandenrodgers left a comment

Choose a reason for hiding this comment

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

🎉

@banderson banderson merged commit 08dfb5f into main Aug 16, 2024
@banderson banderson deleted the ba/package-json-update branch August 16, 2024 17:23
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.

3 participants