Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: move framework-info into build packages#4783

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ericapisani merged 26 commits intomainfromep/add-framework-info
Jan 10, 2023

Conversation

ericapisani
Copy link
Contributor

@ericapisaniericapisani commentedDec 19, 2022•
edited
Loading

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixeshttps://github.com/netlify/pod-ecosystem-frameworks/issues/330

Related issue:https://github.com/netlify/pillar-workflow/issues/1011

We want to eventually merge the functionality withinframework-info intobuild-info since there's a great deal of overlap and having these 2 distinct projects is leading to a high maintenance burden.

This is the first step in this process, which will lead tohttps://github.com/netlify/framework-info/ being archived.

For us to review and ship your PR efficiently, please perform the following steps:

  • Open abug/issue before writing your code 🧑‍💻. This ensures
    we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
    something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read thecontribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@github-actions
Copy link
Contributor

This pull request adds or modifies JavaScript (.js,.cjs,.mjs) files.
Consider converting them to TypeScript.

Copy link
Contributor

@lukasholzerlukasholzer left a comment

Choose a reason for hiding this comment

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

Can you add the same typescript config as for the other libraries that allows js files as well and adapt the files to the dist folders?

ericapisani reacted with thumbs up emoji
Comment on lines 1 to 4
[![npm version](https://img.shields.io/npm/v/@netlify/framework-info.svg)](https://npmjs.org/package/@netlify/framework-info)
[![Coverage Status](https://codecov.io/gh/netlify/framework-info/branch/main/graph/badge.svg)](https://codecov.io/gh/netlify/framework-info)
[![Build](https://github.com/netlify/framework-info/workflows/Build/badge.svg)](https://github.com/netlify/framework-info/actions)
[![Downloads](https://img.shields.io/npm/dm/@netlify/framework-info.svg)](https://www.npmjs.com/package/@netlify/framework-info)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think they need to be updated

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Would you be open to having these just removed outright? Personally I'm not sure how useful they are as statuses here

Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove them :)

needed to add 'root' key in order to support invoking the build command from the repository root
@ericapisani
Copy link
ContributorAuthor

ericapisani commentedDec 20, 2022•
edited
Loading

@lukasholzer

Can you add the same typescript config as for the other libraries that allows js files as well and adapt the files to the dist folders?

At the moment we can't as there are no Typescript files in the package, and if we attempt to add the configuration files now we'll get an error about there being no input files 😞

I'd suggest doing this as a follow up pull request where we convert 1 file to Typescript (even if it's not fully typed) and add the configuration in as part of that

(( edit: Ignore the above, it turned out that if I added.js files as part of theinclude property it removes the error ))

@ericapisaniericapisani marked this pull request as ready for reviewDecember 21, 2022 18:01
{
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"noEmit": true /* Specify an output folder for all emitted files. */
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Vite handles transpilation of TS files by default so this is left to allow for type checking prior to building withvite

jobala
jobala previously approved these changesJan 4, 2023
@ericapisaniericapisani dismissedlukasholzer’sstale reviewJanuary 4, 2023 13:10

Addressed comments and got another code review confirming the changes are good

danez
danez previously requested changesJan 4, 2023
Copy link
Contributor

@danezdanez left a comment

Choose a reason for hiding this comment

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

ericapisani reacted with thumbs up emoji
@danez
Copy link
Contributor

danez commentedJan 5, 2023•
edited
Loading

I thinkrefactor: will not trigger a release-please release, so it should befeat: orfix: when merging, if you want a release

ericapisani and lukasholzer reacted with thumbs up emoji

@ericapisaniericapisani changed the titlerefactor: move framework-info into build packagesfeat: move framework-info into build packagesJan 5, 2023
@ericapisani
Copy link
ContributorAuthor

@danez Good point, I always forget about that

Copy link
Contributor

@lukasholzerlukasholzer left a comment

Choose a reason for hiding this comment

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

small comments

ericapisani added a commit to netlify/framework-info that referenced this pull requestJan 9, 2023
kodiakhqbot pushed a commit to netlify/framework-info that referenced this pull requestJan 10, 2023
* docs: add deprecation noticedepends onnetlify/build#4783* style: lint
@ericapisaniericapisani merged commitd37cbc4 intomainJan 10, 2023
@ericapisaniericapisani deleted the ep/add-framework-info branchJanuary 10, 2023 15:40
jobala pushed a commit that referenced this pull requestJan 11, 2023
Co-authored-by: Lukas Holzer <lukas.holzer@netlify.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@lukasholzerlukasholzerlukasholzer approved these changes

@jobalajobalajobala left review comments

@danezdanezAwaiting requested review from danez

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@ericapisani@danez@lukasholzer@jobala
[8]ページ先頭
©2009-2025 Movatter.jp