Skip to content
This repository has been archived by the owner on Jan 11, 2022. It is now read-only.

Kk/config export #45

Merged
merged 5 commits into from
Dec 13, 2018
Merged

Kk/config export #45

merged 5 commits into from
Dec 13, 2018

Conversation

krzkaczor
Copy link
Contributor

  • move config
  • fix package manifest & build

@GrandSchtroumpf
Copy link
Contributor

Really nice 👍

@holgerd77
Copy link
Member

@krzkaczor Really really great! Didn't expect to have ALL this set of config files included! 😄 👍 I would like to do a short brainstorming round, if having these dependency here on the local node_modules folder, very well that there are none but just have a short thought around this.

I would also like to give other active contributors the chance to have a look:
@vpulim @danjm @alextsg @whymarrh @axic (everyone else invited to comment too):
Hey guys, we are introducing some major changes to the configuration of EthereumJS libraries here. This will be rolled out to most/all the other libraries and should greatly reduce the effort for configuration maintenance and give very much some config consistency between the libs.

Please take the chance and have a look, also on the associated PR of the new config repo coming along with this ethereumjs/ethereumjs-config#1.

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

It might be nice if we can reorganize these changes to not dig into node_modules ourselves. I left a few comments to that effect, though maybe we should open up an issue in ethereumjs/config?

.nycrc Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
tsconfig.prod.json Outdated Show resolved Hide resolved
tslint.json Outdated Show resolved Hide resolved
@krzkaczor
Copy link
Contributor Author

@whymarrh trying to provide module paths was the first thing that I tried. Something like "ethereumjs-config/typescript/tslint.json" this, unfortunately, doesn't work without specifying main file in the package (which we can't do because we would need to specify multiple different entrpoints, tslint config, nyc config etc). So basically I don't see any other way to accomplish it :(

@holgerd77
Copy link
Member

@krzkaczor Can't we use a proxy index.js file which then exposes the different configuration files (optimally also directly considering that there might be more folders/config types then typescript?

@krzkaczor
Copy link
Contributor Author

@holgerd77 interesting idea. I doubt that it will work but i will check it out :D

@whymarrh
Copy link
Contributor

Something like "ethereumjs-config/typescript/tslint.json" [...] doesn't work without specifying main file in the package (which we can't do because we would need to specify multiple different [entrypoints], tslint config, nyc config etc). So basically I don't see any other way to accomplish it :(

I think this is a case for separate packages. It doesn't make sense to have unrelated configurations deployed in the same way. I think they can all live in the same repo and be published as individual npm packages (e.g. @ethereumjs/config-tslint, @ethereumjs/config-nyc, ...). That's what would make the most sense to me.

@holgerd77
Copy link
Member

Would this be a use case for this lerna tool? Stumbled upon that along with all the monorepo discussions, hehe. 😄 Seems to be lightweight enough to also cover simple use cases (like this one), where e.g. no bootstrapping (linking) between dependencies is needed.

If this generally fit this would also be some occasion to generally get in touch with that and collect some experiences (nevertheless @GrandSchtroumpf, no no, no new general monorepo discussion for the moment 😄 😄 😄).

@krzkaczor
Copy link
Contributor Author

hahah yes exactly it sounds like monorepo for config. I know that you love it @holgerd77 😆 I can switch to that approach. I think it will solve all of these issues.

@holgerd77
Copy link
Member

Ok, but without the smiles 😄 do you think it makes sense and is a good fit for this use case (and not e.g. overblown/overengineered)? Then let's do it.

@krzkaczor
Copy link
Contributor Author

@holgerd77 yup I was thinking exactly about that.

As a side note: I created my first monorepo with TS and it's not that great. Tooling sucks (VSCode doesn't navigate between the files correctly, you need to watch files constantly and rebuild type info). So I wouldn't push for it if it doesnt make sense here.

@holgerd77
Copy link
Member

Any way to also centralize the lint, format, test (other?) related script commands and eventually also the devDependencies along? Googled around a bit and stumbled over the explore command from npm, not sure if this applies.

Also thought about moving the dev dependencies to global installation, but this is unrealistic due to in-real-life very like iteratively updated packages on configuration, using different versions at some point in time.

@holgerd77
Copy link
Member

Ah, the explore command is then also running in the scope of the dependency (which probably should have been expected), just tested npm explore ethereumjs-testing -- npm run lint from within the rlp repository (just as a super-random example executing the lint script from the ethereumjs-testing dependency), and this is then doing linting on the dependeny sources.

Nevermind, was just an idea, if someone has some brilliant concept on this please speak out 😄, otherwise we should stick to getting the current scope done.

@krzkaczor
Copy link
Contributor Author

@holgerd77 i never heard about doing anything like that :(

@holgerd77
Copy link
Member

Just created a new typescript branch on the common library, mainly for learning. Already brought the TypeScript errors down from 62 to 61 (the first one always being the most rewarding one 😄 😄)!

Phew, but still have to do a do a google session on every line of code change, but probably that's the way things go. 😄

@holgerd77
Copy link
Member

Down to 2 errors for the main index.js (now: index.ts) transition, this is actually a lot of fun, just pushed: ethereumjs/ethereumjs-common#38

Would be cool if you guys could have a first look.

Not sure if I can really avoid any usage, maybe we have to adjust here and follow the advice from @mattdean-digicatapult and do one in-between-step and loosening type requirements a bit. E.g. the ethereumjs-common library makes heavy use of complex and yet evolving json files and it would be some hell of work to create interfaces for all these dictionary structures.

But maybe I am also just missing something and there is an easier way to achieve this here.

Not sure, eventually we can also stay on the strict side. With the current structure it is possible to override settings, or isn't it? So for libraries where things get to complicated we could then decide case by case.

Anyhow, really great, I am still delighted how much I enjoyed this so far! 🕺 😄

@holgerd77
Copy link
Member

(and already side-caught 3-4 errors along the way)

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks good, feel free to merge if you think it's ready!

@coveralls
Copy link

coveralls commented Dec 13, 2018

Coverage Status

Coverage decreased (-0.1%) to 87.719% when pulling 21b1654 on kk/config-export into 943abb8 on typescript.

"outDir": "./dist"
},
"include": ["src/**/*.ts"]
}
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, thanks, missed that one in my review.

@holgerd77
Copy link
Member

Checked back, all change requests from @whymarrh are resolved, will dismiss his review.

@holgerd77 holgerd77 dismissed whymarrh’s stale review December 13, 2018 11:22

Everything resolved.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks good.

@krzkaczor krzkaczor merged commit f512289 into typescript Dec 13, 2018
@krzkaczor krzkaczor deleted the kk/config-export branch December 13, 2018 11:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants