+
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

feat(npm): allow to set global settings #3123

Merged
merged 3 commits into from
Aug 29, 2022
Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Aug 29, 2022

Summary

Part of #3073

This PR adds a new API to the Node.js called applyConfiguration, which allows a user to apply their own configuration when using the APIs. In this PR, only formatContent is covered.

This won't be the only way a user can offer custom configuration, but it sets the foundation to allow to do so.

The reason why I chose to have a separate function was purely cosmetic. Some option I can see:

  1. Create a new static method called Rome.createWithConfiguration(configuration, backendOption), where configuration is mandatory
  2. Add a new parameter to Rome.create(options, configuration). Although it would lower down the usability because it would unlock something like this Rome.create(undefined, {}), which I don't like to be honest 😛

While doing so, I realised two things:

  • the name of properties passed to applySettings inside the Workspace where in snake case
  • the shape of the object did not reflect the shape of rome.json

This is caused by the fact that the configuration inside rome.json and the configuration inside the Worskpace are different. This is OK, because rome.json needs to offer a more user-friendly shape, while the workspace needs to contain a configuration that suits us developers.

In this PR I made the following changes to the configuration:

  • the function apply_settings inside the server.rs now accepts a Configuration object, which is the user configuration, and it gets mapped using the function merge_with_configuration, which is a function that we already use. This has the effect that now the playground you can see a different shape in the configuration: it's exactly like the rome.json and their properties are in camelCase; I believe it's a great improvement!
  • I added comments to the auto-generated workspace.ts files; this is merely a user experience goody, but it felt weird that we offer a JSON schema in VSCode for our rome.json file, but we don't offer documentation for our configuration in the runtime; the formatted result of the generated code is awful, because our formatter doesn't format comments and get the correct indention is basically impossible. This should not be a show-stopper;

Test Plan

I created a new test

@ematipico ematipico temporarily deployed to aws August 29, 2022 07:39 Inactive
@ematipico ematipico force-pushed the feature/node-api-configuration branch from da31f67 to 408da4b Compare August 29, 2022 07:41
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 29, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a37344f
Status: ✅  Deploy successful!
Preview URL: https://94d8b74c.tools-8rn.pages.dev
Branch Preview URL: https://feature-node-api-configurati.tools-8rn.pages.dev

View logs

@ematipico ematipico temporarily deployed to aws August 29, 2022 07:41 Inactive
@github-actions
Copy link

github-actions bot commented Aug 29, 2022

@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45879 45879 0
Passed 44939 44939 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 5946 5946 0
Passed 1621 1621 0
Failed 4325 4325 0
Panics 0 0 0
Coverage 27.26% 27.26% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12397 12397 0
Failed 3860 3860 0
Panics 0 0 0
Coverage 76.26% 76.26% 0.00%

@ematipico ematipico marked this pull request as ready for review August 29, 2022 07:57
@ematipico ematipico requested a review from leops as a code owner August 29, 2022 07:57
@ematipico ematipico requested a review from a team August 29, 2022 07:57
@ematipico ematipico force-pushed the feature/node-api-configuration branch from 408da4b to 29e0618 Compare August 29, 2022 09:56
@ematipico ematipico temporarily deployed to aws August 29, 2022 09:56 Inactive
@ematipico ematipico force-pushed the feature/node-api-configuration branch from 29e0618 to e4becf7 Compare August 29, 2022 09:57
@ematipico ematipico temporarily deployed to aws August 29, 2022 09:57 Inactive
deserialize_with = "deserialize_globals",
serialize_with = "serialize_globals"
)]
pub globals: IndexSet<String>,
pub globals: Option<IndexSet<String>>,
Copy link
Contributor Author

@ematipico ematipico Aug 29, 2022

Choose a reason for hiding this comment

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

A bug that I caught. The globals was marked as needed. This change fixes the bug.

Comment on lines -63 to +62
Ok(index_set)
Ok(Some(index_set))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change. I moved struct Visitor inside the function deserialize_globals

@ematipico ematipico temporarily deployed to aws August 29, 2022 10:02 Inactive
@ematipico ematipico force-pushed the feature/node-api-configuration branch from 0d7bcea to 9474b79 Compare August 29, 2022 10:31
@ematipico ematipico temporarily deployed to aws August 29, 2022 10:31 Inactive
@ematipico ematipico requested a review from leops August 29, 2022 10:31
@ematipico ematipico force-pushed the feature/node-api-configuration branch from 9474b79 to a37344f Compare August 29, 2022 11:43
@ematipico ematipico temporarily deployed to aws August 29, 2022 11:43 Inactive
@ematipico ematipico merged commit 4a4f985 into main Aug 29, 2022
@ematipico ematipico deleted the feature/node-api-configuration branch August 29, 2022 12:59
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.

2 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载