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

Failure if helper functions are renamed internally in Svelte bundle #538

Closed
Conduitry opened this issue Apr 30, 2017 · 1 comment
Closed

Comments

@Conduitry
Copy link
Member

I've been attempting to upgrade Svelte to Acorn v5 and have been running into interesting issues. On the latest version of Acorn, the assign helper function in the bundle gets renamed to assign$1, because assign conflicts with a new internal assign variable that Acorn is using (for an Object.assign polyfill, incidentally).

This exposed a limitation in the code that lets helper functions depend on other helper functions. In particular, that code expects that

parse( shared[ key ].toString() ).body[0].id.name === key

but this is no longer the case with assign when using Acorn v5.

@Conduitry
Copy link
Member Author

What I have so far is:

diff --git a/src/generators/dom/index.js b/src/generators/dom/index.js
index 173b987..eb768a6 100644
--- a/src/generators/dom/index.js
+++ b/src/generators/dom/index.js
@@ -274,6 +274,13 @@ export default function dom ( parsed, source, options ) {
                        `import { ${names.join( ', ' )} } from ${JSON.stringify( sharedPath )};`
                );
        } else {
+               const namesToKeys = new Map();
+               Object.keys(shared).forEach( key => {
+                       const value = shared[ key ]; // eslint-disable-line import/namespace
+                       if ( typeof value === 'function' ) {
+                               namesToKeys.set( parse( value.toString() ).body[0].id.name, key );
+                       }
+               });
                generator.uses.forEach( key => {
                        const str = shared[ key ].toString(); // eslint-disable-line import/namespace
                        const code = new MagicString( str );
@@ -286,11 +293,11 @@ export default function dom ( parsed, source, options ) {
                                        if ( node._scope ) scope = node._scope;

                                        if ( node.type === 'Identifier' && isReference( node, parent ) && !scope.has( node.name ) ) {
-                                               if ( node.name in shared ) {
+                                               if ( namesToKeys.has( node.name ) ) {
                                                        // this helper function depends on another one
-                                                       generator.uses.add( node.name );
+                                                       generator.uses.add( namesToKeys.get( node.name ) );

-                                                       const alias = generator.alias( node.name );
+                                                       const alias = generator.alias( namesToKeys.get( node.name ) );
                                                        if ( alias !== node.name ) code.overwrite( node.start, node.end, alias );
                                                }
                                        }
@@ -301,7 +308,7 @@ export default function dom ( parsed, source, options ) {
                                }
                        });

-                       const alias = generator.alias( fn.id.name );
+                       const alias = generator.alias( key );
                        if ( alias !== fn.id.name ) code.overwrite( fn.id.start, fn.id.end, alias );

                        builders.main.addBlock( code.toString() );

but this is kind of ugly. Here we're going through all of the shared helper functions first to collect information about which function name maps back to which helper key. Then we walk each used helper function and use this map to figure out what other helpers this uses, instead of just looking at node.name directly. I think doing something like this is going to be necessary, but it can probably be done more tidily.

Rich-Harris added a commit that referenced this issue May 2, 2017
correctly handle when helper functions have been internally renamed (#538)
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

No branches or pull requests

1 participant