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

Feat: default config as dependency #543

Merged
merged 12 commits into from
Apr 18, 2024
Merged

Conversation

tomasciccola
Copy link
Contributor

should close #496 . Depends on this issue to be fixed so that tests can pass.

My approach to vendoring the config was:

  1. add @mapeo/default-config as a devDependency
  2. create a script in scripts/config.js that
    1. looks for the default config file in @mapeo/default by parsing its package.json to get the version (so that we don't hardcode the filename)
    2. copies it into tests/fixtures/config/
  3. add that script as build:config and add it to test

I'm not sure if the default config should be published with core or not (I kinda remember that it shouldn't and that the default config should be a dependency of the frontend separately...)

@tomasciccola tomasciccola marked this pull request as ready for review April 11, 2024 21:45
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM other than my comments.

scripts/config.js Outdated Show resolved Hide resolved
src/config-import.js Outdated Show resolved Hide resolved
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM once we use a "real" @mapeo/default-config.

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

I made a couple of changes and published @mapeo/default-config, so this is ready to merge.

  • When reading dependencies from disk, it's best practice to avoid building the disk path e.g. node_modules/module-name, because there is no guarantee that a dependency is always in the same location, e.g. when this module is installed as a dependency, then any dependencies of this module are normally installed directly in the parent node_modules folder, rather than in the node_modules of this package. As written it probably would have been ok to use what was here, because it was only a dev dep, but to avoid problems in the future I've changed it to use require.resolve.
  • I changed the artifact (built config) filename published by @mapeo/default-config, to remove the version name from the filename. No need to make life difficult for ourselves.
  • I made the artifact the main of @mapeo/default-config, so that we don't need to use the filename at all here - any require of @mapeo/default-config will resolve to the build config file.

@tomasciccola
Copy link
Contributor Author

* I changed the artifact (built config) filename published by `@mapeo/default-config`, to  remove the version name from the filename. No need to make life difficult for ourselves.

😆

* I made the artifact the `main` of `@mapeo/default-config`, so that we don't need to use the filename at all here - any `require` of `@mapeo/default-config` will resolve to the build config file.

Hey this is so cool, I didn't know you could do that with arbitrary assets!

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.

remove bundled config from repo
3 participants