-
-
Notifications
You must be signed in to change notification settings - Fork 737
Feat/639 #1196
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
Feat/639 #1196
Conversation
…less modules double quotes
…dule name after the node_modules folder
…e in case not present
…e in case not present
… the A reflection is already locally declared, exports inside local modules/namespaces need to refer it. In case of library mode, if ReferenceReflection not created, local modules/namespaces were empty without it
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.
I need to pull this down and play with it still, haven't actually seen the output yet. Looks pretty clean for the most part.
@@ -398,6 +398,28 @@ export class Context { | |||
|
|||
return typeParameters; | |||
} | |||
|
|||
/** | |||
* Typscript getFullyQualifiedName method don't always return unique 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.
Yes, that's correct. If a declaration is not exported, it won't contain the source file path... It seems wrong to me to add it since it might conflict with a FQN created by the TS version. That said, I haven't read through everything yet so there might be a good reason for it that I'm not seeing...
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.
In 2 different files I was having the same named export.
And typescript wasn't providing me a unique identifier (no file path in front)
The result was bad reference link in the documentation.
Here in case the fqn doesn't contain a doc, I prefix it with the file path and it has resolved my problem
reflection.name = this.basePath.trim(name); | ||
|
||
// in case of mono-repo (node_modules outside the project), trunk the module name after the node_modules folder | ||
const nodeModulesRegexp = new RegExp('^(.*)node_modules/'); |
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.
Better style to use the literal regex syntax:
const nodeModulesRegexp = new RegExp('^(.*)node_modules/'); | |
const nodeModulesRegexp = /^(.*)node_modules\//; |
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.
I'm kind of worried about this though, it seems like it could lead to paths appearing to be contained within the project that are external.
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.
Ok will fix
@@ -23,6 +23,9 @@ export class DynamicModulePlugin extends ConverterComponent { | |||
*/ | |||
private reflections!: Reflection[]; | |||
|
|||
private hiddenExtension = ['.ts', '.d']; |
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.
Did .ts extensions show up somewhere? I've never seen them...
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.
Ah, I see this is because it now overwrites the name... not sure about this. Will have to play with it.
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.
The goal is to avoid the remaining.d at the end of the file.
It supports the .ts, the .d.ts and classic cases (.js, .jsx, .tsx,...) Thanks to the extension.
But extension alone is not working for .d.ts
Any news for htis branch ? Is is still in progress on your side ? |
new updates here: I resolve all the problematic problems we were having |
Hi what is the status of this PR ? |
Hello Any news ? |
Hey, sorry for the delay in response here. The library mode implementation used in this PR + the feat/639 branch was essentially abandoned. Trying to make library mode fit with the other modes caused problems with maintainability (what was valid in different places changed depending on the mode, which options made sense changed...) I originally thought the way to fix this was going to be rewriting the converters, and started doing that on the library-mode branch... it took too long to realize that would practically never be finished so I took another look at the feat/639 branch + this branch, this resulted in the work on the library2 branch, which I'll be putting a beta up for this weekend. I don't plan on merging either of these branches, but it has been very useful in working on the library2 branch, so thanks for that! |
I will take a look at library2 branch to see. |
If you run into issues with the library2 branch - by all means open an issue or a pull request! I've set a deadline this time around. Library mode will be released by the end of the year. The beauty of open source is that forking is possible - so long as you abide by the license I have absolutely no problems with forks, though of course I'd like to make the implementation in library2 so good that nobody feels the need to fork to use another implementation :) (Note: library2 is published under |
0.20 has been released, thank you for your work here :) |
correct the default exports and star imported namespaces in library mode