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

Browser #361

Merged
merged 2 commits into from
Jul 12, 2018
Merged

Browser #361

merged 2 commits into from
Jul 12, 2018

Conversation

cancerberoSgx
Copy link
Contributor

@cancerberoSgx cancerberoSgx commented Jul 12, 2018

Fix #354

  • wrap globby require call the same way as with fs-extra so it supports the browser
  • npm run overwrite-declaration-file : replace copy /Y with shx cp so it support non-windows OS (needed to install shelljs).

See example: https://gist.github.com/cancerberoSgx/2151d205c4aa13a7973cf25dd8e396d6

Feedback :

  • useVirtualFileSystem will work in the browser - no need to implement FileSystemHost from scratch
  • Missing documentation - I could write a couple of paragraphs (see example bottom comments) but I'm not sure in which section, perhaps in filesystem's?
  • Would be good idea to export VirtualFileSystemHost so users can override a couple of them to support, for ex, indexDB or reuse it even for other techs besides the browser. Document its methods stating that async ones are based on syncs (so users override only those) and also change modifiers of some attributes (directories) from private to protected so subclasses have visibility
  • I've seen some libraries in similar situation (eslint) distribute a browser bundle or provide npm run build-browser script. is easy to do but I'm not sure is worthwhile for ts-simple-ast since I need to add browserify or webpack devDependencies
  • tests ? (again IMO not worthwhile )
  • future : better dependency injection - DefaultFileSystemHost is required and used in some places directly (LanguageServiceHost). Probably this could be easily solved with a factory

@dsherret dsherret merged commit 4b57519 into dsherret:master Jul 12, 2018
@dsherret
Copy link
Owner

@cancerberoSgx Awesome! Thanks so much!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.348% when pulling 34d9f19 on cancerberoSgx:browser into ee85f10 on dsherret:master.

@dsherret
Copy link
Owner

@cancerberoSgx btw, I really appreciate the cross platform change you did in the script. I had just done that lazily as a temporary solution because I didn't know the best way to do that (you seem to have picked something good).

Also, this is released now in 12.5.3. Let me know if you have any issues.

@dsherret dsherret added the bug label Jul 12, 2018
@dsherret
Copy link
Owner

Btw, I'll read your feedback section on the weekend. Please ping me in case I forget for some reason, but I've set a reminder to do so.

@cancerberoSgx
Copy link
Contributor Author

cancerberoSgx commented Jul 13, 2018

@dsherret I only worried about documenting this a somehow, but I don't want to pollute current docs with this that's kind of OT. I would do the following: 1) add this file (or similar) somewhere in the repos (https://gist.github.com/cancerberoSgx/2151d205c4aa13a7973cf25dd8e396d6) . 2) add a section titled "Browser support" with small paragraph and a link to that file .3) Not sure where that section could be added. What do you think ?

dsherret pushed a commit that referenced this pull request May 14, 2019
fix: Do not import globby unless using the file system (browser support).
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 this pull request may close these issues.

None yet

3 participants