Support for generating SVG components without `.defaultProps` #129

Grohden · 2024-12-05T14:45:55Z

This should close #126

I've changed the code to:

Use inline 'defaultProps' in the generated SVG components
Use object.assign to merge the prop overrides (see linked issue)
Use an option (emitDeprecatedDefaultProps) that allows this package users to still keep emitting the defaultProp declarations

I've made the option to omit default props an opt-out, so it is a breaking change.. but up for the author to change that to a default false

…ect assign instead

Grohden · 2024-12-05T14:47:06Z

Oh... btw I couldn't test (in a real project), ever since I've opened this issue we've migrated away from babel & this plugin

ljharb · 2024-12-06T06:58:23Z

src/index.js

+          if (!path.scope.hasBinding('object.assign/implementation')) {
+            const assignDeclaration = t.importDeclaration([
+              t.importDefaultSpecifier(t.identifier('objectAssign')),
+            ], t.stringLiteral('object.assign/implementation'));
+
+            file.set('ensureObjectAssign', () => {
+              const [newPath] = path.unshiftContainer('body', assignDeclaration);
+              newPath.get('specifiers').forEach((specifier) => { path.scope.registerBinding('module', specifier); });
+            });
+          } else {
+            file.set('ensureObjectAssign', () => {});
+          }


ahhhh i think i misunderstood :-/ we definitely can't inject a package dependency into people's code (that we don't already know is there, like react)

oh well... do we have any idea on how to do it without assign? 😟

I tested letting just spread in the code, for some instances babel did transform it and used helper methods

i'm not sure we can produce spread and rely on babel transpiling it in user codebases

ljharb · 2024-12-06T06:58:43Z

src/index.js

@@ -138,6 +158,20 @@ export default declare(({
          if (typeof filename === 'undefined' && typeof opts.filename !== 'string') {
            throw new TypeError('the "filename" option is required when transforming code');
          }
+
+          if (!path.scope.hasBinding('object.assign/implementation')) {


if we did this, we'd want to use /polyfill and invoke that, so that we use the native version when present and compliant

but like, I don't think we want to polyfill people envs right?

of course not :-) that's what "shim" does. "polyfill" is just a function that gets the best possible version.

oh, ok, I'll change the code to use shim instead.. I went with the impl one cuz I can just call it, for shim I'll have to either somehow inject this expr:

var objectAssignShim = require('object.assign/shim'); // it probably needs to be separated for cases where we generate `import` syntax... 🤔 but if you think it is really safe to call require(...)(); I'm up for it var objectAssign = objectAssignShim(); var Svg = function(overrides) { var props = objectAssign({}, DEFAULTS, overrides); // .... }

or make it like this, which is the easy way to do it...

var objectAssignShim = require('object.assign/shim'); var Svg = function(overrides) { var props = objectAssignShim()({}, DEFAULTS, overrides); // .... }

I'll make it like this ^, I don't really know the implications of that objectAssignShim() call on every render.. but I don't think it will be simple to inject that var objectAssign = objectAssignShim() at the top levels

Made the change

sorry if i was unclear; shim is the one we DONT want to use. polyfill is the one we DO want to use, because it will be the native one when possible.

ohhh, sorry for misunderstanding, I thought polyfill would polyfill the env 😆 (now what I read bout shim makes more sense) Ill make the changes tomorrow

made the change to use polyfill

src/index.js

Grohden · 2024-12-06T12:41:06Z

test/sanity.js

  validateDefaultProps(result);
-  console.log('test/fixtures/test-multiple-svg.jsx', result.code);
+  console.log('test/fixtures/test-multiple-svg.jsx\n', result.code);


So we know, I've thought it was clearer to read the outputs with a \n, but if you want I can revert this

src/index.js

ljharb · 2024-12-06T18:48:43Z

src/index.js

+    const defaultProps = SVG_DEFAULT_PROPS_CODE
+      ? 'var props = objectAssign({}, SVG_DEFAULT_PROPS_CODE, overrides);'
+      : '';
+    const PROPS_NAME = SVG_DEFAULT_PROPS_CODE ? 'overrides' : 'props';


where does "overrides" come from? is that a react thing?

Nop, thats just renamed props, I didn't want to change the body (SVG_CODE) so I made a little rename here, the final function will be

var SVG_NAME = function SVG_NAME(overrides) { var props = objectAssign({}, SVG_DEFAULT_PROPS_CODE, overrides); return SVG_CODE // which will probably ref `props`, which I didn't want to mess with so overrides is just a rename }

Grohden · 2024-12-07T20:32:19Z

src/index.js

  }) => {
+    const defaultProps = SVG_DEFAULT_PROPS_CODE
+      ? 'var props = objectAssign({}, SVG_DEFAULT_PROPS_CODE, overrides);'


btw, one implication of assign here is that just props will never be a stable ref, but I don't think anything in the SVG_CODE will ref just props alone so it should be ok...

Grohden · 2024-12-07T20:36:15Z

src/index.js

+      ignorePattern,
+      caseSensitive,
+      filename: providedFilename,
+      emitDeprecatedDefaultProps = false,


@ljharb I mentioned in the PR description, but JIC, this is a breaking change if this is default false... especially if someone reflects on the SVG default props

I think that since react went with the warnings this is a good thing (no warnings out of the box experience), but as a lib consumer I also don't love breaking changes 😞 though IMO, just reading the change log should do it 🤷

ljharb · 2024-12-07T22:31:19Z

To clarify, SVG_DEFAULT_PROPS_CODE is opts.SVG_DEFAULT_PROPS_CODE = t.objectExpression(defaultProps);, and so we can probably just remove that internal option. defaultProps is created from iterating over the actual SVG code, and reading the properties therein, so we could instead make like defaultEntries - an array of 2-tuples, property name and value, and then inside the component, iterate over that and mutate props.

However, a much much simpler approach is probably to just make this a class component and keep using defaultProps, since I'm pretty sure nothing about class components here would be deprecated?

Grohden · 2024-12-07T22:40:53Z

@ljharb thats actually... a really good solution, just hope that whatever env people have supports classes 🤔 (~97% of web?)

Ill explore classes in this PR then

ljharb · 2024-12-07T22:42:03Z

ah, good point. I think we can make a class component without class syntax though.

Grohden · 2024-12-14T12:35:27Z

@ljharb I've setup a nextjs project with babel to test this PR changes and essentially I've discovered that I can't use class components in nextjs server side

Error:   × You're importing a component that needs `PureComponent`. This React hook only works in a client component. To fix, mark the file (or its parent) with the `"use client"` directive.
  │ 
  │  Learn more: https://nextjs.org/docs/app/api-reference/directives/use-client

Essentially, due to nextjs and SSR (and potentially any other SSR impl?) if we want to emit class components people should mark their files as client components, which is a pretty stupid requirement to be able to use inline SVGs... So I guess we should stick to the fn components idea

Grohden · 2024-12-14T12:51:53Z

@ljharb made the change to polyfill and tested it in my mentioned nextjs project

About this comment I'm not really sure I want to mutate props, its unlikely that people will be relying on a props object after passing it to a component but 🤷 on top of that it will make things more complicated for me in this PR

ljharb · 2024-12-14T16:45:21Z

@Grohden class components can extend React.PureComponent though?

Grohden · 2024-12-15T18:46:11Z

@ljharb yes (actually, isn't it the "only" way to make class component?), but will cause that problem I've mentioned on next and potentially any SSR solution out there 🤔

ljharb · 2024-12-15T20:01:53Z

No, class extends React.Component is a normal class component, and class extends React.PureComponent is a pure component. Functional components can't be "pure". SSR has worked fine for the better part of a decade, even before SFCs existed, so I don't think it's an SSR problem - but it indeed might be a next.js-specific one. Regardless of what we do here, can you file an issue on next? Any valid React component should be usable anywhere any kind of react component is accepted.

refactor: make default props emission disabled by default and use obj…

0361226

…ect assign instead

ljharb requested changes Dec 6, 2024

View reviewed changes

ljharb marked this pull request as draft December 6, 2024 07:00

Grohden commented Dec 6, 2024

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

chore: remove unneeded empty interpolation

0a0a078

ljharb reviewed Dec 6, 2024

View reviewed changes

chore: reformat template args

10bc8e2

Grohden commented Dec 7, 2024

View reviewed changes

refactor: change implementation to use shims

237f618

refactor: change to object.assign polyfill

50ad04b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for generating SVG components without `.defaultProps` #129

Support for generating SVG components without `.defaultProps` #129

Grohden commented Dec 5, 2024 •

edited

Loading

Grohden commented Dec 5, 2024 •

edited

Loading

ljharb Dec 6, 2024

Grohden Dec 6, 2024

Grohden Dec 6, 2024

ljharb Dec 6, 2024

ljharb Dec 6, 2024

Grohden Dec 6, 2024

ljharb Dec 6, 2024

Grohden Dec 7, 2024

Grohden Dec 7, 2024

ljharb Dec 7, 2024

Grohden Dec 7, 2024

Grohden Dec 14, 2024

Grohden Dec 6, 2024

ljharb Dec 6, 2024

Grohden Dec 7, 2024

Grohden Dec 7, 2024

Grohden Dec 7, 2024

ljharb commented Dec 7, 2024

Grohden commented Dec 7, 2024

ljharb commented Dec 7, 2024

Grohden commented Dec 14, 2024

Grohden commented Dec 14, 2024

ljharb commented Dec 14, 2024

Grohden commented Dec 15, 2024

ljharb commented Dec 15, 2024

Support for generating SVG components without .defaultProps #129

Are you sure you want to change the base?

Support for generating SVG components without .defaultProps #129

Conversation

Grohden commented Dec 5, 2024 • edited Loading

Grohden commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Dec 7, 2024

Grohden commented Dec 7, 2024

ljharb commented Dec 7, 2024

Grohden commented Dec 14, 2024

Grohden commented Dec 14, 2024

ljharb commented Dec 14, 2024

Grohden commented Dec 15, 2024

ljharb commented Dec 15, 2024

Support for generating SVG components without `.defaultProps` #129

Support for generating SVG components without `.defaultProps` #129

Grohden commented Dec 5, 2024 •

edited

Loading

Grohden commented Dec 5, 2024 •

edited

Loading