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

Include simple config objects when extracting static plugins #14699

Conversation

philipp-spiess
Copy link
Member

@philipp-spiess philipp-spiess commented Oct 17, 2024

This PR updates the extractStaticPlugins function to also emit options as long as these are objects containing of only string and number values.

While doing this I also cleaned up the require('custom-plugin') detector to use a Tree-Sitter query instead of operating on the AST.

Here are the two cases we considered:

import plugin1 from 'plugin1';

export default {
  plugins: [
    plugin1({
      foo: 'bar',
      num: 19,
    }),
    require('./plugin2')({
      foo: 'bar',
      num: 19,
    }),
  ]
}

The test plan also contains a number of scenarios that we do not want to migrate to CSS (because we do not have a CSS API we can use for e.g. nested objects). We do support all types that we also support in the CSS API.

Copy link
Member Author

philipp-spiess commented Oct 17, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @philipp-spiess and the rest of your teammates on Graphite Graphite

@philipp-spiess philipp-spiess force-pushed the 10-17-include_simple_config_objects_when_extracting_static_plugins branch 2 times, most recently from 20d0c57 to 438c487 Compare October 17, 2024 13:22
@philipp-spiess philipp-spiess marked this pull request as ready for review October 17, 2024 13:27
@philipp-spiess philipp-spiess requested a review from a team as a code owner October 17, 2024 13:27
@philipp-spiess philipp-spiess force-pushed the 10-17-include_simple_config_objects_when_extracting_static_plugins branch 2 times, most recently from aab02ca to 09c0aab Compare October 17, 2024 13:41
@philipp-spiess philipp-spiess force-pushed the 10-17-include_simple_config_objects_when_extracting_static_plugins branch from 09c0aab to 3c0528a Compare October 17, 2024 14:51
}
`),
export default {
plugins: [falserequire('./plugin1')]
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads like a typo rather than an intentional non-require call. Maybe like load or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha I had it call frequire before and I actually went in and fixed it since I thought it's a typo 🤕 agree i will just call it load so there's absolutely no confusion.

@adamwathan
Copy link
Member

adamwathan commented Oct 17, 2024

Should we add integration tests for this? It feels wrong to me that I can't look at any tests and see "given this config file, the equivalent generated CSS file is this".

Edit: Ah I think that's probably covered in the follow-up PR.

@thecrypticace
Copy link
Contributor

thecrypticace commented Oct 17, 2024

Should we add integration tests for this? It feels wrong to me that I can't look at any tests and see "given this config file, the equivalent generated CSS file is this".

Those are in the followup PR where the actual CSS changes happen: #14700

@philipp-spiess philipp-spiess force-pushed the 10-17-include_simple_config_objects_when_extracting_static_plugins branch from d775256 to 34fc9b0 Compare October 18, 2024 11:08
Copy link
Member Author

philipp-spiess commented Oct 18, 2024

Merge activity

  • Oct 18, 9:10 AM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Oct 18, 9:10 AM EDT: A user merged this pull request with Graphite.

@philipp-spiess philipp-spiess merged commit 3e7695f into next Oct 18, 2024
2 checks passed
@philipp-spiess philipp-spiess deleted the 10-17-include_simple_config_objects_when_extracting_static_plugins branch October 18, 2024 13:10
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.

3 participants