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

[@eas/config] add library #3

Merged
merged 2 commits into from
Sep 17, 2020
Merged

Conversation

wkozyra95
Copy link
Contributor

No description provided.

@wkozyra95 wkozyra95 requested a review from fson September 14, 2020 15:10
@@ -8,6 +9,7 @@ export default class Login extends Command {

async run() {
this.log('Log in to EAS');
await new EasJsonReader(process.cwd(), 'all').readAsync('release');
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 is just to make sure that imports work

@wkozyra95 wkozyra95 force-pushed the @wkozyra95/add-eas-json-support branch from 330ed67 to 650c07d Compare September 15, 2020 10:55
Copy link
Contributor

@fson fson left a comment

Choose a reason for hiding this comment

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

Thanks! The overall structure of the library seems fine. 👍

Added a few other comments about the draft PR below.


jest.mock('fs-extra');

describe('eas.json', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can omit the describe block and indentation and put tests in the top level of this file, as there is only one group of tests in this file.

private validateBuildProfile<T extends BuildProfile>(
platform: 'android' | 'ios' | 'all',
buildProfileName: string,
buildProfile?: { workflow: Workflow } & object
Copy link
Contributor

Choose a reason for hiding this comment

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

You could define an interface to avoid using & object.

"bugs": "https://github.com/expo/eas-cli/issues",
"dependencies": {
"@expo/build-tools": "^0.1.16",
"@hapi/joi": "^17.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to be careful with the dependencies we add to EAS CLI so that we don't end up with the same problems as with Expo CLI, which has grown really big. The Joi library seems quite large for just doing object validation (https://packagephobia.com/result?p=%40hapi%2Fjoi), so maybe we can see if there's a more lightweight alternative that we can use.

However, I'm not sure what's the best alternative, https://github.com/vriad/zod is one TypeScript library that I know of, which also can infer the TypeScript types of objects so we don't end up defining everything twice. Maybe see if it would be fit for our purposes? It's almost 50% smaller 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.

@expo/build-tools is also using joi internally, rewriting that would require significant changes in turtle code.

For now, build tools contains code that implements all the build process on the worker, but in the future, we will extract types and schemas to separate package.

Copy link
Contributor Author

@wkozyra95 wkozyra95 Sep 15, 2020

Choose a reason for hiding this comment

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

If size is a big concern maybe ensuring that we don't include multiple versions of the same lib is more important. Maybe some common package with wrappers around those libraries ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's important to not include multiple versions of the same lib, but we also need to be careful about choosing libraries, because many library authors don't pay a lot of attention to their dependencies, which makes the library grow big.

#7 will help with this.

@fson fson changed the base branch from master to main September 16, 2020 09:08
@wkozyra95 wkozyra95 force-pushed the @wkozyra95/add-eas-json-support branch from 650c07d to 0ddca6e Compare September 16, 2020 10:10
@wkozyra95 wkozyra95 marked this pull request as ready for review September 16, 2020 10:12
@wkozyra95 wkozyra95 requested a review from fson September 16, 2020 10:12
testEnvironment: 'node',
testRegex: '/__tests__/.*(test|spec)\\.[jt]sx?$',
transform: {
'^.+\\.[jt]sx?$': ['babel-jest', { configFile: require.resolve('./babel.config.js') }],
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this and the babel config to the root jest/ folder so it can be reused across modules and the babel.config.js won't need to be in each package individually.

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 just copied that from eas-cli. I don't really know what would be a better choice here, I leave the decision on that to @fson

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, now that we have two packages let's make them use the same config.

@@ -0,0 +1,3 @@
# @eas/config

A library for interacting with the eas.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Some thoughts from working on expo/config:

  1. the main reason we added support for app.config.js was because people wanted to configure the publishing and building step dynamically.
  2. We have support for TS/ES support in app.config.js but this is problematic because it makes the package really large and it only works for the config file itself, so if someone were to import a package, that package would need to be transpiled already. So if we do opt for dynamic support I recommend not adding TS/ES support.

Copy link
Contributor Author

@wkozyra95 wkozyra95 Sep 16, 2020

Choose a reason for hiding this comment

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

I named it config for consistency, but we don't support ts/js, it's only json

@wkozyra95 wkozyra95 force-pushed the @wkozyra95/add-eas-json-support branch from adc1c22 to 2537c36 Compare September 17, 2020 12:52
@wkozyra95 wkozyra95 force-pushed the @wkozyra95/add-eas-json-support branch from 2537c36 to dcffb2e Compare September 17, 2020 12:53
@wkozyra95 wkozyra95 merged commit 728a73d into main Sep 17, 2020
@wkozyra95 wkozyra95 deleted the @wkozyra95/add-eas-json-support branch October 14, 2020 13:52
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

3 participants