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

fix: various app-builder issues #5055

Merged
merged 16 commits into from Sep 24, 2021
Merged

fix: various app-builder issues #5055

merged 16 commits into from Sep 24, 2021

Conversation

joshbalfour
Copy link
Contributor

Pull request checklist

Detail as per issue below (required):

fixes: #

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Closing this pull request since the title does not match ^(?:(?:[WIP] ?)?(?:build|ci|chore|docs|feat|fix|perf|refactor|revert|style|test):(?:\ +?#\d+?\ +?)?.)|(?:[Snyk].) pattern. Please fix the title and re-open the pull request.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Closing this pull request since the title does not match ^(?:(?:[WIP] ?)?(?:build|ci|chore|docs|feat|fix|perf|refactor|revert|style|test):(?:\ +?#\d+?\ +?)?.)|(?:[Snyk].) pattern. Please fix the title and re-open the pull request.

@joshbalfour joshbalfour changed the title bugfix: various app-builder issues fix: various app-builder issues Sep 20, 2021
@joshbalfour joshbalfour reopened this Sep 20, 2021
@@ -9,6 +8,7 @@ export const slugToCamel = (str) =>
const capitalize = (s) => s && s[0].toUpperCase() + s.slice(1)

export const lint = async (code: string): Promise<string> => {
const baseEslint = require('@reapit/ts-scripts/src/eslint/base-eslint') // inline import because ts-scripts has dependency issues
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this is just because you need to have the eslint executable in scope in your package - can explain in the morning!

twitter: false,
yandex: false,
windows: false,
const webpackConfigDev = ({ appName }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels a bit scary because this is used by all the things - bit hard to follow what changed - can you just fill me in in the morning please? Ta!

Copy link
Contributor

@bashleigh bashleigh left a comment

Choose a reason for hiding this comment

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

Only one small suggestion

packages/app-builder-backend/src/eject/templates/app.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@willmcvay willmcvay left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@joshbalfour joshbalfour merged commit 5e18736 into master Sep 24, 2021
@joshbalfour joshbalfour deleted the feat/demo-fixes branch September 24, 2021 10:42
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.

None yet

3 participants