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

refactor(config): load the config as a JS module instead of parsing it as JSON #2599

Closed
wants to merge 8 commits into from

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented May 9, 2022

Basic work for continuing with #2577

@changeset-bot
Copy link

changeset-bot bot commented May 9, 2022

🦋 Changeset detected

Latest commit: 73b28db

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@commercetools-frontend/application-config Patch
@commercetools-frontend/application-shell Patch
@commercetools-frontend/cypress Patch
@commercetools-frontend/mc-html-template Patch
@commercetools-frontend/mc-scripts Patch
merchant-center-application-template-starter Patch
playground Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented May 9, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
merchant-center-application-kit ✅ Ready (Inspect) Visit Preview May 10, 2022 at 2:51PM (UTC)

const result = get(moduleExport, 'default', moduleExport);

// Write the application config to `stdout`so that the main program can read it.
process.stdout.write(JSON.stringify(result));
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this was executed as a Node script. The config was then printed to stdout and parsed from the child process.

});

// Require the module. It's expected that the module exports the application config
const moduleExport = require(filePath);
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we simply require the JS module, no JSON serialization.

This allows to define and preserve JS functions into the application config (useful for example for dev server plugins)

@vercel vercel bot temporarily deployed to Preview May 9, 2022 20:11 Inactive
@emmenko emmenko force-pushed the nm-config-load-as-module branch from 8fff87e to 59929db Compare May 10, 2022 08:14
@vercel vercel bot temporarily deployed to Preview May 10, 2022 08:20 Inactive
@emmenko emmenko marked this pull request as draft May 10, 2022 08:55
@vercel vercel bot temporarily deployed to Preview May 10, 2022 09:03 Inactive
@vercel vercel bot temporarily deployed to Preview May 10, 2022 13:41 Inactive
@vercel vercel bot temporarily deployed to Preview May 10, 2022 14:44 Inactive
@emmenko emmenko force-pushed the nm-config-load-as-module branch from 4b7d889 to 73b28db Compare May 10, 2022 14:47
@vercel vercel bot temporarily deployed to Preview May 10, 2022 14:51 Inactive
@emmenko
Copy link
Member Author

emmenko commented May 10, 2022

So this is not going anywhere, as I don't like any of the approaches I tried.

The main driver for this was to be able to define functions within the config and be able to use them. For example, for Vite to pass extra plugins and use them when starting the dev server.

I'm opting now for a different approach, to have plugins defined in a separate file and simply reference the file path in the custom app config.

Small downside is that there would be another file but I think it's a reasonable tradeoff.

@emmenko
Copy link
Member Author

emmenko commented May 16, 2022

Closing as there would be a different approach.

@emmenko emmenko closed this May 16, 2022
@emmenko emmenko deleted the nm-config-load-as-module branch May 16, 2022 14:26
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.

1 participant