-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
1. resolves the path to the config (by also grabbing its version) 2. copies it to dist/
…efaultConfigAsDependency
There was a problem hiding this 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.
Co-authored-by: Evan Hahn <me@evanhahn.com>
There was a problem hiding this 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
.
There was a problem hiding this 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 parentnode_modules
folder, rather than in thenode_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 userequire.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 - anyrequire
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! |
should close #496 . Depends on this issue to be fixed so that tests can pass.
My approach to vendoring the config was:
@mapeo/default-config
as a devDependencyscripts/config.js
that@mapeo/default
by parsing its package.json to get the version (so that we don't hardcode the filename)tests/fixtures/config/
build:config
and add it totest
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...)