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

Predictable module names (fixes #469) #485

Merged
merged 1 commit into from
Oct 17, 2022
Merged

Conversation

oyvindberg
Copy link
Collaborator

@oyvindberg oyvindberg commented Oct 10, 2022

Problem

It's almost impossible to map Javascript module names to the currently generated ST object/package names in your head.

There is a 1-1 correspondence, but it's impossible to understand. Let's finally fix this mistake!

I know this will lead to "uglier"/longer module names, this has been discussed various times in the past, but the encoding needs to become easier to understand.

Keep old behavior

add stShortModuleNames (for sbt plugin) and --shortModuleNames for CLI which reverts to old behaviour.

Note that this old behaviour is deprecated. I don't intend to remove it for a long while, but I won't put any effort into maintaining it either.

Example

Here is a typical example of what it'll look like - see how clearly you can see the hierarchy even though it's flattened.

Screenshot 2022-10-13 at 14 57 55

@oyvindberg oyvindberg force-pushed the predictable-module-names branch 5 times, most recently from 6c2d592 to b15041b Compare October 13, 2022 12:50
@oyvindberg oyvindberg force-pushed the predictable-module-names branch from b15041b to e91b838 Compare October 13, 2022 12:51
@oyvindberg
Copy link
Collaborator Author

One possible problem is that this might trigger windows long file name errors, those were somewhat common in the past. I think if that happens let's rather compress the file names on windows and keep the package names correct.

add `stShortModuleNames` (for sbt plugin) and `--shortModuleNames` for CLI which reverts to old behaviour.

Note that the old behaviour is deprecated
@oyvindberg oyvindberg force-pushed the predictable-module-names branch from e91b838 to 317b19f Compare October 17, 2022 23:47
@oyvindberg oyvindberg merged commit 2ae7282 into master Oct 17, 2022
@oyvindberg oyvindberg deleted the predictable-module-names branch October 17, 2022 23:52
@nafg
Copy link
Contributor

nafg commented Oct 21, 2022

With class files the packages will always correspond to directories so not sure that is an option

@oyvindberg
Copy link
Collaborator Author

@nafg to gauge the problem I wrote a script to traverse all the jar files for a full CI build, looking for the longest class files. The longest is a trait produced from the fake string literals:

typings/octokitPluginRestEndpointMethods/octokitPluginRestEndpointMethodsStrings$DELETE$u0020SlashreposSlashLeftcurlybracketownerRightcurlybracketSlashLeftcurlybracketrepoRightcurlybracketSlashactionsSlashcachesLeftcurlybracketQuestionmarkkeyCommarefRightcurlybracket.class

This will blow up the windows path budget in any case, weighing in at 274 chars. On the bright side, this should be gone after #487, so let's dig up the longest class class file name after removing those of fake strings:

typings/officeUiFabricReact/libComponentsPickersPeoplePickerPeoplePickerItemsPeoplePickerItemDottypesMod$IPeoplePickerItemSelectedSubComponentStyles$IPeoplePickerItemSelectedSubComponentStylesMutableBuilder$.class

214 chars. Will very likely fail on windows, given that there is a semi-long prefix added to this.

If we change the naming pattern for the builder class, it could come out like this instead:

typings/officeUiFabricReact/libComponentsPickersPeoplePickerPeoplePickerItemsPeoplePickerItemDottypesMod$IPeoplePickerItemSelectedSubComponentStyles$MutableBuilder$.class

That's 171 chars. borderline possible to compile on windows.

@oyvindberg
Copy link
Collaborator Author

Then there are anonymous traits, object types not named in TS.

typings/reactNativePaper/anon/CallHasLabelAccessibilityLabelPageNumberOfPagesOnPageChangeStyleThemeShowFastPaginationControlsNumberOfItemsPerPageListNumberOfItemsPerPageOnItemsPerPageChangeSelectPageDropdownLabelSelectPageDropdownAccessibilityLabelRest.class

260 chars. may also want to enforce length of these traits, without introducing name collisions. I thought that was already in place honestly, but there's probably a bug in that code

oyvindberg added a commit that referenced this pull request Oct 23, 2022
…o not have a name.

This is a follow-up from #485, to avoid too long class names on windows
oyvindberg added a commit that referenced this pull request Oct 23, 2022
…o not have a name.

This is a follow-up from #485, to avoid too long class names on windows
oyvindberg added a commit that referenced this pull request Oct 23, 2022
…Builder`.

It should be fine, since they are always nested within the companion object, and they should not be named anyway in user code.

This only applies to scala 2.x, these are rewritten to `extension` for scala 3.

This is a follow-up from #485, to avoid too long class names on windows
oyvindberg added a commit that referenced this pull request Oct 23, 2022
…o not have a name.

This is a follow-up from #485, to avoid too long class names on windows
oyvindberg added a commit that referenced this pull request Oct 23, 2022
…Builder`.

It should be fine, since they are always nested within the companion object, and they should not be named anyway in user code.

This only applies to scala 2.x, these are rewritten to `extension` for scala 3.

This is a follow-up from #485, to avoid too long class names on windows
@oyvindberg oyvindberg mentioned this pull request Nov 14, 2022
@FabioPinheiro
Copy link
Contributor

Thanks, @oyvindberg for pointing out where the changes come from.
I tested the flag and stShortModuleNames keeps all the code compiling with no change (on my project).
Either way, I will probably change the code and not use the flag.

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