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

Common package refactor #237

Merged
merged 30 commits into from
Jul 26, 2022
Merged

Common package refactor #237

merged 30 commits into from
Jul 26, 2022

Conversation

benja
Copy link

@benja benja commented Jul 18, 2022

This pull request introduces NPM workspaces to the repository. The list of packages are as follows: @eik/common-classes, @eik/common-helpers, @eik/common-schemas and @eik/common-validators.

What you must do

  • If you're using ReadFile or EikConfig, you must now install this from @eik/common-classes
  • If you're using addTrailingSlash, removeTrailingSlash, addLeadingSlash, removeLeadingSlash, isStream, isReadableStream, typeSlug or typeTitle, you must now install this from @eik/common-helpers
  • If you're using schema, validate, assert or ValidationError, you must now install this from @eik/common-schemas
  • If you're using any of the validators (origin, org, name, type, etc.), you must now install this from @eik/common-validators

Thoughts

I'd say before we go ahead and merge this, we should attempt a publish and try and port over some logic from other internal packages using common to see if it's working as expected.

@benja benja marked this pull request as ready for review July 20, 2022 14:06
@trygve-lie
Copy link
Contributor

Great work!

I am very happy with @eik/common-validators, @eik/common-schemas and @eik/common-helpers being separate packages. Maybe we could name @eik/common-helpers into @eik/common-utils instead. What do you think?

For @eik/common-classes I think we still need to do a little bit more work. I've poked a bit around in the code and how we use whats in this package and I'm not sure @eik/common-classes is a suitable name. Most of the code in here now is all about loading the eik config and providing an eik config object one can read values from, and save to disc. Maybe @eik/common-config-loader is a more suitable name. What do you think?

What I really struggle with in this repo (previous to this rework) is to know what the public API is and I still struggle a little bit with that in the current state of @eik/common-classes. I've looked through most of the modules using files from this repo and what I find is:

  • All the build tool plugins and the node client is only the getDefaults() method in the get-defaults.js file. This method tries to find a eik config and if so loads it and provides a eik config Object.

  • The @eik/cli module uses the getDefaults() method, configStore method and the EikConfig class. In other words, the only interesting parts to expose in the public API is getDefaults(), configStore and EikConfig.

This brings me the conclusion that I think we should try to structure @eik/common-classes (or what ever we rename this module too) as follow to begin with:

|- src
   |- index.js (exposes the public API)
   |- get-defaults.js
   |- config-store.js
   |- eik-config.js
   |- classes
     |- read-file.js
     |- file-mapping.js
     |- local-file-location.js
     |- remote-file-location.js
     |- resolved-files.js
   |- errors
     |- custom-error.js
     |- invalid-config-error.js
     |- missing-config-error.js
     |- multiple-config-sources-error.js
     |- no-files-matched-error.js
     |- single-dest-multiple-source-error.js
   |- utils
     |- resolve-files.js 

Then there is a wild card in here: local-assets.js. This file contain a set of http routes for serving the content of configStore in Express, Fastify and Hapi. I am not sure where this are in use, but I feel it does not belong in this module at all as is. Maybe it should live in the @eik/node-client module or even in a separate http module. What do you think?

@benja
Copy link
Author

benja commented Jul 25, 2022

Thanks for great feedback 🚀

I've changed two of the package names and updated the structure for @eik/common-config-loader. Let me know what you think of it now.

I'm not sure if it's worth creating a whole new repo just for local-assets.js as of now, but it would make more sense to do so as we have more HTTP utilities. As of now, we can either keep it in the common module or move it to @eik/node-client as that appears to be more appropriate for HTTP utils.

@benja benja changed the base branch from master to next July 26, 2022 07:54
@benja
Copy link
Author

benja commented Jul 26, 2022

PR is ready for review before merging into next, @trygve-lie.

@trygve-lie
Copy link
Contributor

I think this looks great for now.

Happy to have local-assets.js on root as a first step. Lets move it to a more appropriate location later.

Lets do a merge to next and cut some next releases.

@benja benja merged commit c6f9613 into next Jul 26, 2022
@benja benja deleted the refactor branch July 26, 2022 10:28
@github-actions
Copy link

🎉 This PR is included in version 4.0.0-next.6 🎉

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.

None yet

2 participants