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

IGAPP-602: Setup basic ts project web #267

Merged
merged 11 commits into from
May 4, 2021

Conversation

sarahsporck
Copy link
Contributor

This pull request belongs to an issue on our bugtracker.
You can find it there by looking for an issue with the key which is mentioned in the title of this pull request.
It starts with the keyword IGAPP.

  • I already copied the www package so the fonts mentioned in index.ejs could be found without any issues
  • I could not import the build config yet, as the files do not compile on this branch. I think it makes sense to first do IGAPP-597 before adding them to the webpack.config.js

Copy link
Member

@maxammann maxammann left a comment

Choose a reason for hiding this comment

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

Before I start reviewing this, should we base this on main?

@sarahsporck
Copy link
Contributor Author

Before I start reviewing this, should we base this on main?

Hmm. Not Sure about that as it seems the build config files already have a *.ts ending, but don't compile. This would definitely create issue with the ci. Or did you mean changing the base branch to Main without any change from web-ts-init?

@steffenkleinle
Copy link
Member

I think the latter, otherwise reviewing is quite hard if you don't remember what we were doing on the conference already here.

@ztefanie
Copy link
Member

I think the latter, otherwise reviewing is quite hard if you don't remember what we were doing on the conference already here.

Should web-ts-init also be merged? Should we open a PR for it?

@steffenkleinle
Copy link
Member

I'd just review it together as web-ts-init is really WIP and not finished (or rather: finished by this branch/PR). So I think we should just review and merge it together in this PR.

@sarahsporck sarahsporck force-pushed the IGAPP-602-setup-basic-ts-project-web branch from a59ef30 to 1601880 Compare April 28, 2021 14:04
@sarahsporck sarahsporck changed the base branch from IGAPP-web-ts-init to main April 28, 2021 14:05
@sarahsporck
Copy link
Contributor Author

sarahsporck commented Apr 28, 2021

Some files have been autogenerated by create-react-app (README.md, App.css, ...) I guess. Should I remove them or keep them for now?

Furthermore I recommend checking this branch out and clicking around. A lot of the changes still encompass the www folder

"npm": ">=3.8"
},
"scripts": {
"start": "cross-env TS_NODE_PROJECT=\"tools/tsconfig.webpack.json\" webpack-dev-server --config tools/webpack.config.ts --progress --env.config_name=integreat-test-cms --env.dev_server",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cross-env TS_NODE_PROJECT=\"tools/tsconfig.webpack.json\" most ugly workaround for webpack.config.ts
Read more here in the webpack-docu or this webpack-cli issue

web-ts/package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,295 @@
import {Configuration, DefinePlugin, LoaderOptionsPlugin, optimize} from "webpack";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay typescript 🎉

Copy link
Member

Choose a reason for hiding this comment

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

Yay, can we have prettier/linting as well? 😁 Or is this something we should rather do in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Separate PR I think. We also do not have it in the flow project.

Copy link
Member

Choose a reason for hiding this comment

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

Why would we want it in the flow project?

@@ -0,0 +1,27 @@
{
"compilerOptions": {
"target": "ES2020",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one we could discuss. On IGAPP-web-ts-init it was set to esnext, which is at least in my opinion not a stable form of release. In our current web project we use es6(=es2015) syntax.

Copy link
Member

Choose a reason for hiding this comment

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

I think ES2020 is fine for now 👍

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this config will support IE11, but I'd just leave this and see some time later what to choose here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean here. Probably we will have to stay with es6, as previously. Check out Caniuse.com.

@steffenkleinle
Copy link
Member

Some files have been autogenerated by create-react-app (README.md, App.css, ...) I guess. Should I remove them or keep them for now?

Furthermore I recommend checking this branch out and clicking around. A lot of the changes still encompass the www folder

I think you could remove those files, less to review for us and less changes at a later point.

@steffenkleinle
Copy link
Member

🎉 🎉

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Seems to work 👍 Highway to typescript 🚀

web-ts/src/index.ejs Show resolved Hide resolved
web-ts/package.json Outdated Show resolved Hide resolved
web-ts/tools/tsconfig.webpack.json Outdated Show resolved Hide resolved
web-ts/README.md Outdated Show resolved Hide resolved
web-ts/package.json Outdated Show resolved Hide resolved
web-ts/src/index.css Outdated Show resolved Hide resolved
web-ts/src/index.ejs Show resolved Hide resolved
web-ts/src/reportWebVitals.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,295 @@
import {Configuration, DefinePlugin, LoaderOptionsPlugin, optimize} from "webpack";
Copy link
Member

Choose a reason for hiding this comment

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

Separate PR I think. We also do not have it in the flow project.

web-ts/tools/webpack.config.ts Show resolved Hide resolved
web-ts/tools/tsconfig.webpack.json Outdated Show resolved Hide resolved
@@ -0,0 +1,27 @@
{
"compilerOptions": {
"target": "ES2020",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this config will support IE11, but I'd just leave this and see some time later what to choose here.

web-ts/www/humans.txt Outdated Show resolved Hide resolved
@sarahsporck sarahsporck enabled auto-merge May 4, 2021 09:44
@sarahsporck sarahsporck merged commit fa24a0e into main May 4, 2021
@sarahsporck sarahsporck deleted the IGAPP-602-setup-basic-ts-project-web branch May 4, 2021 10:19
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.

4 participants