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

feat: Re-enable typescript for production builds #328

Merged
merged 2 commits into from
Apr 14, 2023

Conversation

marlonkeating
Copy link
Contributor

@marlonkeating marlonkeating commented Apr 5, 2023

This is a fixed version of #294 (reverted in #315), with package.json that includes typescript libraries in dependencies instead of devDependencies, which ensures that the requires typescript libraries will get pulled into any consumers of openedx/frontend-build.

Testing Done

Example App

  • npm run build succeeds
  • npm run test succeeds

Local MFE

using typescript pulling frontend-build but no typescript libraries

  • npm run build succeeds
  • npm run test succeeds

@abdullahwaheed
Copy link
Contributor

Thank you @marlonkeating for updating this and fixing the issues. Since now we have separate repo for typescript-config, i think we should move all the typescript related configurations in that repo and just update the respective changes here.
cc: @adamstankiewicz

@adamstankiewicz adamstankiewicz changed the base branch from master to alpha April 13, 2023 14:12
tsconfig.json Outdated Show resolved Hide resolved
example/src/Image.tsx Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@adamstankiewicz adamstankiewicz force-pushed the mkeating/fix-production-typescript branch from 84556bd to a59fb3e Compare April 13, 2023 16:18
@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (b1d43c7) 0.00% compared to head (84556bd) 0.00%.

❗ Current head 84556bd differs from pull request most recent head 10782df. Consider uploading reports for the commit 10782df to get more accurate results

Additional details and impacted files
@@     Coverage Diff      @@
##   alpha   #328   +/-   ##
============================
============================

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@marlonkeating marlonkeating force-pushed the mkeating/fix-production-typescript branch 2 times, most recently from a94c929 to 6280e10 Compare April 13, 2023 22:20
@adamstankiewicz adamstankiewicz force-pushed the mkeating/fix-production-typescript branch from 6280e10 to 5cddfbd Compare April 14, 2023 12:03
Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

CI is failing on npm run lint due to omitted .tsx file extensions. I believe we need to modify our ESLint config to add TypeScript support so it doesn't complain about things like this.

This can be achieved with airbnb-typescript, but would recommend adding it to our shared @edx/eslint-config library, given that's where our ESLint config extends airbnb and airbnb/hooks already.

See this PR: openedx/eslint-config#124

@marlonkeating marlonkeating force-pushed the mkeating/fix-production-typescript branch from 5cddfbd to 55fa392 Compare April 14, 2023 20:18
Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

LGTM to merge into alpha, though with a clarifying question first about the typescript dependency and a few nits.

package.json Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
example/tsconfig.json Show resolved Hide resolved
example/src/Image.tsx Outdated Show resolved Hide resolved
build: Add .ts/.tsx extensions to webpack resolver

chore: Add ending newline to Image.tsx

build: Use typescript config from @edx/eslint-config

chore: formatting
@marlonkeating marlonkeating force-pushed the mkeating/fix-production-typescript branch from 55fa392 to adb7300 Compare April 14, 2023 21:34
@marlonkeating marlonkeating merged commit 8e64e9e into alpha Apr 14, 2023
@marlonkeating marlonkeating deleted the mkeating/fix-production-typescript branch April 14, 2023 23:07
@edx-semantic-release
Copy link

🎉 This PR is included in version 12.9.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 12.10.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 13.1.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 15.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

5 participants