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

Constants declared by using object/array binding pattern syntax fail the build #315

Closed
thefat32 opened this issue Apr 14, 2024 · 9 comments · Fixed by #317
Closed

Constants declared by using object/array binding pattern syntax fail the build #315

thefat32 opened this issue Apr 14, 2024 · 9 comments · Fixed by #317
Assignees
Labels
Milestone

Comments

@thefat32
Copy link

Bug report

When importing "single-spa" package (in this case "single-spa-react" is importing "single-spa") dts-bundle-generator fails with error:

image

Input code

import React from "react";
import ReactDOMClient from "react-dom/client";
import singleSpaReact from "single-spa-react";

const lifecycles = singleSpaReact({
  React,
  ReactDOMClient,
  rootComponent: () => "HELLO WORLD!",
  errorBoundary(err, info, props) {
    console.error("Error Boundary", err, info, props);
    return React.createElement('div', { className: 'root' }, 'ERROR BOUNDARY');
  },
});

export const { bootstrap, mount, unmount } = lifecycles;

Expected output

Not to fail

Actual output

Fails with Error: Cannot find symbol for node...

Additional context

Resproduction repo:

https://github.com/thefat32/dts-bundle-generator-single-spa-bug

The error is beacuse of this export syntax in node_modules/single-spa/typings/single-spa.d.ts

  export const {
    NOT_LOADED = "NOT_LOADED",
    LOADING_SOURCE_CODE = "LOADING_SOURCE_CODE",
    NOT_BOOTSTRAPPED = "NOT_BOOTSTRAPPED",
    BOOTSTRAPPING = "BOOTSTRAPPING",
    NOT_MOUNTED = "NOT_MOUNTED",
    MOUNTING = "MOUNTING",
    MOUNTED = "MOUNTED",
    UPDATING = "UPDATING",
    UNMOUNTING = "UNMOUNTING",
    UNLOADING = "UNLOADING",
    SKIP_BECAUSE_BROKEN = "SKIP_BECAUSE_BROKEN",
    LOAD_ERROR = "LOAD_ERROR",
  };

In single-spa's repo

It's a strange syntax I have never seen it, but it seems to be interpreted correctly by typescript. As an axample this typescript code compiles without erros in typescript:

import { NOT_LOADED } from "single-spa";

console.log(NOT_LOADED);

// prints "NOT_LOADED"
@timocov
Copy link
Owner

timocov commented Apr 14, 2024

The error is beacuse of this export syntax in node_modules/single-spa/typings/single-spa.d.ts

Omg what the hell is this... I don't know what the compiler even allows to write it this way, this looks like a bug and shouldn't be allowed 😄

Anyway, I'll take a look what we can do to avoid an error from happening (but no promises, this syntax is very exotic and I need to play with it a bit to see what else could be there apart from just const values).

@timocov timocov added the Bug label Apr 14, 2024
@thefat32
Copy link
Author

At first I thought it was a bug in the single-spa.d.ts file... I don't know why typescript compiler is allowing this syntax. Couldn't find any info about it.

I've created an issue in single-spa's repo to ask them to stop using this exotic/bugged syntax.

single-spa/single-spa#1215

@timocov
Copy link
Owner

timocov commented Apr 14, 2024

I also asked in the typescript discord chat about this syntax, will see what people would say...

@timocov timocov added this to the 9.4 milestone Apr 15, 2024
@timocov timocov changed the title Error: Cannot find symbol for node... when importing single-spa Constants declared by using object/array binding pattern syntax fail the build Apr 15, 2024
@timocov
Copy link
Owner

timocov commented Apr 15, 2024

There is a non-zero chance that this cannot be supported by the tool (without writing a ton of code). Currently I'm leaning towards putting a warning if the tool encountered such syntax. The build won't fail with the error above, but the resulted dts might or might not be correct in this case (which will be warned by the tool ofc). The only possible solution would be to change the source code.

From my testing I couldn't find a way how the compiler would generate such code (any object/array binding is converted into "variable declaration per a variable" syntax which is much easier to parse and I believe it is handled already).

@thefat32
Copy link
Author

I think single-spa.d.ts file is being updated manually by the single-spa team. That's why it has that weird syntax.

@thefat32
Copy link
Author

After playing around with the typescript compiler it seems like declaring that syntax is equivalent in every way to expand it to multiple export const definitions.

I'm not related with the code in this tool but I think detecting this kind of node and expanding it to multiple nodes should fix the problem. As an example

Node:

export const {
    string = "string"
    number = 2,
    array = [1,2,3,4],
    object = { number: 2, func: () => ({})},
    promise= Promise<any>,
    func = () => ({})
}

Should be expanded to multiple nodes:

export const string = "string";
export const number = 2;
export const array = [1,2,3,4];
export const object = { number: 2, func: () => ({})};
export const promise= Promise<any>;
export const func = () => ({});

@timocov
Copy link
Owner

timocov commented Apr 15, 2024

I think detecting this kind of node and expanding it to multiple nodes should fix the problem

I wish it was just so easy to do... Btw apart from splitting a variable declaration list into multiple variable declarations this is exactly what the compiler does when you write similar code but in .ts file.

The issue is, the tool doesn't transform source files nor even their compiled versions. In order to perform any transformation the only option I'm aware of requires to run a compilation with transformation, but the issue is - the compiler doesn't transform .d.ts files because they are treated as "output" already thus not being transformed. Furthermore, such declaration can be in node_modules which is ignored from output generation regardless.

I might have a solution tho - it looks like the only option is to "print" source file and parse it again, which might and most likely will have performance implications (we can try to reduce it, but it still would have some cost to check when we need to do it).

@timocov timocov modified the milestones: 9.4, 9.5 Apr 15, 2024
@timocov timocov self-assigned this Apr 15, 2024
timocov added a commit that referenced this issue Apr 16, 2024
@timocov
Copy link
Owner

timocov commented Apr 17, 2024

The fix has been published in v9.5.0.

@thefat32
Copy link
Author

@timocov impressive work, thank you very much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants