This repository was archived by the owner on Aug 31, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 653
feat(npm): allow to set global settings #3123
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
da31f67
to
408da4b
Compare
Deploying with
|
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 |
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
408da4b
to
29e0618
Compare
29e0618
to
e4becf7
Compare
ematipico
commented
Aug 29, 2022
deserialize_with = "deserialize_globals", | ||
serialize_with = "serialize_globals" | ||
)] | ||
pub globals: IndexSet<String>, | ||
pub globals: Option<IndexSet<String>>, |
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.
A bug that I caught. The globals
was marked as needed. This change fixes the bug.
ematipico
commented
Aug 29, 2022
Comment on lines
-63
to
+62
Ok(index_set) | ||
Ok(Some(index_set)) |
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.
This is the only change. I moved struct Visitor
inside the function deserialize_globals
leops
reviewed
Aug 29, 2022
0d7bcea
to
9474b79
Compare
9474b79
to
a37344f
Compare
leops
approved these changes
Aug 29, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, onlyformatContent
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:
Rome.createWithConfiguration(configuration, backendOption)
, whereconfiguration
is mandatoryRome.create(options, configuration)
. Although it would lower down the usability because it would unlock something like thisRome.create(undefined, {})
, which I don't like to be honest 😛While doing so, I realised two things:
applySettings
inside theWorkspace
where in snake caserome.json
This is caused by the fact that the configuration inside
rome.json
and the configuration inside the Worskpace are different. This is OK, becauserome.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:
apply_settings
inside theserver.rs
now accepts aConfiguration
object, which is the user configuration, and it gets mapped using the functionmerge_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 therome.json
and their properties are in camelCase; I believe it's a great improvement!workspace.ts
files; this is merely a user experience goody, but it felt weird that we offer a JSON schema in VSCode for ourrome.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