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(templates): remove description, license & name keys from package.json #3287

Merged

Conversation

MichaelDeBoey
Copy link
Member

As mentioned by @pcattori in #3221 (comment), these fields aren't necessary for apps.

@pcattori
Copy link
Contributor

sideEffects is actually required for bundling to work as expected. Setting sideEffects: false in an app's package.json marks all imports from that app (e.g. relative imports) as side-effect free.

@MichaelDeBoey MichaelDeBoey force-pushed the remove-unnecessary-package-json-keys branch from 7ab6493 to c77e9d6 Compare May 22, 2022 17:28
@MichaelDeBoey MichaelDeBoey changed the title chore(templates): remove description, license & sideEffects keys from package.json chore(templates): remove description & license keys from package.json May 22, 2022
@MichaelDeBoey
Copy link
Member Author

@pcattori I've added sideEffects back to package.json 👍

@pcattori
Copy link
Contributor

@MichaelDeBoey name should also be removed.

@MichaelDeBoey
Copy link
Member Author

@pcattori Since each app has a certain name, I think we should keep name for convenience.

@pcattori
Copy link
Contributor

pcattori commented May 22, 2022

@pcattori Since each app has a certain name, I think we should keep name for convenience.

I'm not sure I follow. What convenience does the name field provide?

@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented May 22, 2022

If I'm not mistaken (@mcansh can probably provide more info about this), integration(?) tests will also fail if examples/templates have the same/no name 🤔
Hence the patchup-example-names script.

@pcattori
Copy link
Contributor

pcattori commented May 22, 2022

If I'm not mistaken (@mcansh can probably provide more info about this), integration(?) tests will also fail if examples/templates have the same/no name 🤔 Hence the patchup-example-names script.

If we already have a script for handling name collisions, that's a smell that name isn't the right thing for the job in the first place. We should instead just use the filename or filepath in the patchup-example-names script to populate the name field dynamically for the examples/tests. That would guarantee uniqueness (due to filesystem), whereas the name field does not prevent duplicate names.

@MichaelDeBoey
Copy link
Member Author

Thinking about this a bit more and maybe we should just keep these fields, as npm will warn the following otherwise 🤔

npm WARN example-app No description
npm WARN example-app No repository field.
npm WARN example-app No license field.

@pcattori
Copy link
Contributor

Thinking about this a bit more and maybe we should just keep these fields, as npm will warn the following otherwise 🤔

npm WARN example-app No description
npm WARN example-app No repository field.
npm WARN example-app No license field.

Does that app have private: true?

@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented May 22, 2022

Does that app have private: true?

@pcattori It was indeed missing that one, my bad 😅🙈

@MichaelDeBoey
Copy link
Member Author

I'm not sure I follow. What convenience does the name field provide?

@pcattori People are used to have a name in all their package.jsons, so I think we'll get PRs to add it (again) if we decide to remove it.

create-react-app & create-next-app also add name to package.json btw.
https://github.com/facebook/create-react-app/blob/c20ccecfa1cf130a47d37908dc54959c618ce8ea/packages/create-react-app/createReactApp.js#L266-L270
https://github.com/vercel/next.js/blob/863db9b0e2a7b038348202c040f90f050fd12855/packages/create-next-app/create-app.ts#L202-L212

@mcansh
Copy link
Collaborator

mcansh commented May 22, 2022

tests won't fail due to naming conflicts, we'll just get bombarded with jest haste warnings because of them. i haven't tried with no names, so i can't say for sure if it's fine without them

@pcattori
Copy link
Contributor

pcattori commented May 22, 2022

I'm not sure I follow. What convenience does the name field provide?

@pcattori People are used to have a name in all their package.jsons, so I think we'll get PRs to add it (again) if we decide to remove it.

create-react-app & create-next-app also add name to package.json btw. https://github.com/facebook/create-react-app/blob/c20ccecfa1cf130a47d37908dc54959c618ce8ea/packages/create-react-app/createReactApp.js#L266-L270 https://github.com/vercel/next.js/blob/863db9b0e2a7b038348202c040f90f050fd12855/packages/create-next-app/create-app.ts#L202-L212

I don't think we'll get PRs to add it back in. People can just add their app name if they want to their app's package.json. We don't have an issue with that. But the template itself shouldn't include a name.

create-react-app and create-next-app also set name to be the app name, not the template name.


Nothing requires name or is more convenient (AFAIK) because of name, so we should omit it.

For those who like having a name field defined for their app, they are welcome to add one. They would have needed to edit it anyway if we had the template name set as the name, so omitting the name is not any more onerous than what we currently have (the template name).

@MichaelDeBoey
Copy link
Member Author

They would have needed to edit it anyway if we had the template name set as the name, so omitting the name not any more onerous than what we currently have (the template name).

Good point!
Would it be a good idea to automatically add the app name as name instead?

@pcattori
Copy link
Contributor

pcattori commented May 22, 2022

They would have needed to edit it anyway if we had the template name set as the name, so omitting the name not any more onerous than what we currently have (the template name).

Good point! Would it be a good idea to automatically add the app name as name instead?

I think its better to leave name out. Keeps create-remix simpler and makes it clear that its not required.

@MichaelDeBoey MichaelDeBoey force-pushed the remove-unnecessary-package-json-keys branch from c77e9d6 to e5ddd60 Compare May 22, 2022 23:23
@MichaelDeBoey MichaelDeBoey changed the title chore(templates): remove description & license keys from package.json chore(templates): remove description, license & name keys from package.json May 22, 2022
@MichaelDeBoey MichaelDeBoey force-pushed the remove-unnecessary-package-json-keys branch from e5ddd60 to aa98d52 Compare May 22, 2022 23:24
@pcattori pcattori merged commit d5e58d3 into remix-run:main May 23, 2022
@MichaelDeBoey MichaelDeBoey deleted the remove-unnecessary-package-json-keys branch May 23, 2022 01:58
eastlondoner pushed a commit to eastlondoner/remix that referenced this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants