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

EDTR: TypeScript support. #1838

Merged
merged 16 commits into from Nov 13, 2019
Merged

EDTR: TypeScript support. #1838

merged 16 commits into from Nov 13, 2019

Conversation

nicolad
Copy link
Contributor

@nicolad nicolad commented Nov 12, 2019

Implements this issue: #1836

tn3rb
tn3rb previously approved these changes Nov 12, 2019
Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

beautiful! love it!

think there's one output config line that looks like it still needs deleting.
going to approve now assuming the above will be fixed

@@ -138,7 +124,6 @@ const config = [
module: moduleConfigWithJsRules,
output: {
filename: '[name].[chunkhash].dist.js',
Copy link
Member

Choose a reason for hiding this comment

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

should this be removed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely, thx.

EDTR Conversion to Apollo GraphQL. branch: EDTR/master automation moved this from In progress to Reviewer approved Nov 12, 2019
EDTR Conversion to Apollo GraphQL. branch: EDTR/master automation moved this from Reviewer approved to Review in progress Nov 12, 2019
@nicolad nicolad marked this pull request as ready for review November 12, 2019 16:52
@tn3rb tn3rb self-requested a review November 12, 2019 17:14
@mnelson4
Copy link
Contributor

Just heard of TypeScript today, and it sounds like our jam. But let's not forget to document these changes both for our own reference (including the client-side Ludites like me who think we're basically moving back to client-side Java applets), and also for 3rd party devs who may not be familiar with TypeScript or our usage of it.

We don't need to give a tutorial on TypeScript of course, but IMO we should state why we're using it, give some links to docs on how to learn more, and make sure someone who's new to TypeScript can still setup EE from source and run it locally.

@nicolad
Copy link
Contributor Author

nicolad commented Nov 12, 2019

Just heard of TypeScript today, and it sounds like our jam. But let's not forget to document these changes both for our own reference (including the client-side Ludites like me who think we're basically moving back to client-side Java applets), and also for 3rd party devs who may not be familiar with TypeScript or our usage of it.

We don't need to give a tutorial on TypeScript of course, but IMO we should state why we're using it, give some links to docs on how to learn more, and make sure someone who's new to TypeScript can still setup EE from source and run it locally.

Sure, is this location appropriate for this purpose: https://github.com/eventespresso/event-espresso-core/tree/master/docs/AA--Javascript-in-EE ?

tn3rb
tn3rb previously approved these changes Nov 12, 2019
@@ -21,7 +23,7 @@ const DropDownMenu = ( {
tooltipPosition = 'top left',
index = 0,
...otherProps
} ) => {
} : PropTypes.InferProps<typeof DropDownMenu.propTypes> ) => {
Copy link
Member

Choose a reason for hiding this comment

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

this is going to conflict with my branch that changes the folder structure...
although I might have to rework that branch to avoid having a gazillion broken unit tests { : \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a problem.
the power of sublime merge will help me 🛠️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tn3rb I had reverted this change since it was causing tests failure and orchestrating all the stuff to handle tsx for jest needs to be done in a separate PR.


The only thing left for now in this PR is docs, as suggested by @mnelson4

EDTR Conversion to Apollo GraphQL. branch: EDTR/master automation moved this from Review in progress to Reviewer approved Nov 12, 2019
@mnelson4
Copy link
Contributor

mnelson4 commented Nov 12, 2019

Sure, is this location appropriate for this purpose: /docs/AA--Javascript-in-EE@master ?

🤘 Yeah I think somewhere in there. But I'm not sure if it's best to add a new file, or new folder of files, or just update existing files. I suppose it kinda depends on the nature of the changes.

EDTR Conversion to Apollo GraphQL. branch: EDTR/master automation moved this from Reviewer approved to Review in progress Nov 12, 2019
@nicolad nicolad force-pushed the EDTR/ts branch 2 times, most recently from eea5a5f to 780f060 Compare November 13, 2019 08:38
@nicolad nicolad requested a review from tn3rb November 13, 2019 11:10
@nicolad nicolad mentioned this pull request Nov 13, 2019
@nicolad nicolad force-pushed the EDTR/ts branch 2 times, most recently from 0d0fdb1 to fef3704 Compare November 13, 2019 12:42
Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

awesome!

think there's one spot that's still referencing the bin folder that needs to be updated

Comment on lines +285 to +287
const enhancedConfig = config
.map( enhance )
.map( addLoaders );
Copy link
Member

Choose a reason for hiding this comment

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

very nice 👍

const common = require( './webpack.common.js' );
const webpack = require( 'webpack' );
const miniExtract = require( 'mini-css-extract-plugin' );
const wpi18nExtractor = require( './bin/i18n-map-extractor.js' );
const wpi18nExtractor = require( '../bin/i18n-map-extractor.js' );
Copy link
Member

Choose a reason for hiding this comment

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

bin folder got renamed no?

Copy link
Contributor Author

@nicolad nicolad Nov 13, 2019

Choose a reason for hiding this comment

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

Not yet, I moved only asset-dependency-maps.js from ./bin directory for now, because this file was related to webpack configs.
Didn't wanted to pollute PR's diff with too many changes.
If you want, I can move the other files and references from ./bin to ./config

Copy link
Member

Choose a reason for hiding this comment

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

ahh ok, what you have done is fine.

Ideally we should move all config files into the new config folder you created, but that can happen in another PR.
I don't understand why that bin folder was created considering it does not contain any binary files !?!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe bin from trash bin/can 🤔

Comment on lines +1 to +10
## Static type checking with `TypeScript`.

TypeScript compilation is integrated into build process via webpack, through TS loaders. This means that we don't have to run any additional scripts. By running usual webpack-related scripts all TS compilation will be done automatically.

Since we already have some codebase written in JS, in `tsconfig.json` the option `allowJs` has been set to `true`. This will enable us to make an incremental adoption of TypeScript.

Related external reading:

- [React & Webpack](https://www.typescriptlang.org/docs/handbook/react-&-webpack.html)
- [Migrating from JavaScript](https://www.typescriptlang.org/docs/handbook/migrating-from-javascript.html)
Copy link
Member

Choose a reason for hiding this comment

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

good stuff 💯

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@tn3rb tn3rb self-requested a review November 13, 2019 15:35
manzoorwanijk
manzoorwanijk previously approved these changes Nov 13, 2019
EDTR Conversion to Apollo GraphQL. branch: EDTR/master automation moved this from Review in progress to Reviewer approved Nov 13, 2019
const ASSETS = 'assets';

const assetsManifestOutputPath = path.resolve( ASSETS, 'dist/build-manifest.json' );
const assetsPath = `./${ ASSETS }/ZZZ/`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added const here for consistency, but I am not 💯 happy with how readability ended :)

Copy link
Member

Choose a reason for hiding this comment

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

oh ha ha ya we don't want to keep that as all of the new EDTR replacement files will still be in /assets/src/
the ZZZ folder was just a way to keep all of the existing logic untouched and out of the way while we refactor everything

EDTR Conversion to Apollo GraphQL. branch: EDTR/master automation moved this from Review in progress to Reviewer approved Nov 13, 2019
@nicolad nicolad merged commit 980996c into EDTR/master Nov 13, 2019
EDTR Conversion to Apollo GraphQL. branch: EDTR/master automation moved this from Reviewer approved to Done Nov 13, 2019
@nicolad nicolad deleted the EDTR/ts branch November 13, 2019 17:50
@manzoorwanijk manzoorwanijk removed their assignment Nov 14, 2019
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.

None yet

4 participants