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

Support AMD modules in Salsa #6942

Closed
egamma opened this issue Feb 6, 2016 · 16 comments
Closed

Support AMD modules in Salsa #6942

egamma opened this issue Feb 6, 2016 · 16 comments
Labels
Domain: JavaScript The issue relates to JavaScript specifically In Discussion Not yet reached consensus Suggestion An idea for TypeScript VS Code Tracked There is a VS Code equivalent to this issue

Comments

@egamma
Copy link
Member

egamma commented Feb 6, 2016

From @justin-romano on February 3, 2016 21:56

It just does nothing unless its a local variable.
I would have suspected this would present a list of all possible matching items in the source.
Its almost as if its not even indexing the code. I am working in a source base of millions of lines of JS so it would be nice.
Products like resharper and visual assist do this better.

Copied from original issue: microsoft/vscode#2683

@egamma egamma added the js label Feb 6, 2016
@egamma
Copy link
Member Author

egamma commented Feb 6, 2016

From @dresoy on February 4, 2016 1:17

I agree, it should search it for JavaScript.

@egamma
Copy link
Member Author

egamma commented Feb 6, 2016

@egamma
Copy link
Member Author

egamma commented Feb 6, 2016

From @justin-romano on February 5, 2016 20:53

Thanks egamma but i could not see it make a difference. Is there some think like lucene indexing the code when you load a folder?
It does not seem very busy when it starts and noticed that the search is really slow which implies its just trawling the files rather then indexing the source.

@egamma
Copy link
Member Author

egamma commented Feb 6, 2016

@justin-romano Then I assume your code isn't using the commonjs module system? Are you using another JS module system?

@egamma
Copy link
Member Author

egamma commented Feb 6, 2016

From @justin-romano on February 6, 2016 16:50

No actually anguar and requirejs.

On Sun, Feb 7, 2016 at 4:52 AM, Erich Gamma [email protected]
wrote:

@justin-romano https://github.com/justin-romano Then I assume your code
isn't using the commonjs module system? Are you using another JS module
system?


Reply to this email directly or view it on GitHub
microsoft/vscode#2683 (comment).

Justin Romaine
Senior Systems Architect
Spark Dental Technology
[email protected]
ph 021 764 506
hm 09 445 9166

@egamma
Copy link
Member Author

egamma commented Feb 6, 2016

Thanks, got it. Salsa doesn't support AMD yet. I'll move it as a feature request to Salsa component.

@egamma egamma removed the js label Feb 6, 2016
@mhegazy mhegazy changed the title Go to Definition does not work when using AMD Support AMD modules in Salsa Feb 6, 2016
@mhegazy mhegazy added Suggestion An idea for TypeScript Revisit An issue worth coming back to labels Feb 6, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Feb 6, 2016

When we looked into this, AMD seemed to be fairly complicated to get right statically, at least compared to CommonJS modules.

@mhegazy mhegazy added In Discussion Not yet reached consensus and removed Revisit An issue worth coming back to labels Feb 6, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Feb 6, 2016

@egamma can you weigh in on the priority of AMD module support? given that ES6 is now ubiquitous, as well as other bundling technologies like browserify and webpack that use CommonJS.

@mhegazy mhegazy added the Salsa label Feb 6, 2016
@justin-romano
Copy link

Why dont you just have a grab everything from everywhere mode. So you don't
need a dependency change. Just scan the folders.
Goto definition then should just give you a choice rather that you can
scroll based on most likely candidates. Also remembers your previous
choices and places them at the top of the list. kind of like the ctrl+P
works

On Sun, Feb 7, 2016 at 9:57 AM, Mohamed Hegazy [email protected]
wrote:

@egamma https://github.com/egamma can you weigh in on the priority of
AMD module support? given that ES6 is now ubiquitous, as well as other
bundling technologies like browserify and webpack that use CommonJS.


Reply to this email directly or view it on GitHub
#6942 (comment)
.

Justin Romaine
Senior Systems Architect
Spark Dental Technology
[email protected]
ph 021 764 506
hm 09 445 9166

@egamma
Copy link
Member Author

egamma commented Feb 7, 2016

given that ES6 is now ubiquitous, as well as other bundling technologies like browserify and webpack that use CommonJS.

@mhegazy I fully agree that this is correct for more recent JS code bases, but there are also code bases with more mileage. I'll try to find out whether there is some data on this.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 7, 2016

given that ES6 is now ubiquitous

Syntax yes, but there is yet to be any real loader or agreement on module resolution! 😄 Which is largely what is holding a lot of things back in my opinion.

There are significant issues with CommonJS in the browser, which is exactly why AMD came about to deal with those challenges (and had to split from CommonJS because there couldn't be agreement), to allow asynchronous lazy loading, two pass instantiation, etc. etc. and those advantages and AMD builders like r.js are heavily used by many real life projects in lieu alternatives like Browserify and WebPack. In face, though popularity contests aren't always an indicator, RequireJS out strips Browserify and WebPack and only recently has WebPack overtaken RequireJS.

I do agree though, static analysis is challenging for AMD modules, as there are several valid scenarios and because AMD by its nature is two pass instantiation, with circular references and potentially complex configuration information being available to the loader that makes mapping of the MID to the actual file challenging, it wouldn't be a trivial task to accomplish.

@regrettably
Copy link

regrettably commented Feb 22, 2017

The lack of AMD support makes me unable to use VSCode, unfortunately, since I work on a codebase that is a couple of years old and still uses AMD/RequireJS. I don't really want to use IntelliJ, but it's able to do analysis of my code far more trivially than VSCode is able to...

@MystK
Copy link

MystK commented Mar 8, 2017

I'd like to add that AMD support would be super helpful in VSCode.

I'm a Netsuite developer, and it uses AMD syntax. It makes it harder when I can't easily click a definition and go to it when other IDEs like WebStorm can. Instead, I have to do a search all manually.

@MankalMR
Copy link

MankalMR commented Mar 8, 2017

I would definitely love to have to this feature. I think there are several legacy codebases out there which would benefit from this feature.
One way to resolve the dependencies can be to have the vscode lookup a requires config file (this can be configurable per project).

@prantlf
Copy link

prantlf commented Dec 23, 2017

If you are looking for the support of AMD/CJS JavaScript modules for the "Go to Definition" command (custom definition provider), you can have a look at the VS Code extension "Require Module Support":

It supports RequireJS configuration (paths) to look up and jump to the original location of the selected identifier's definition.

@mjbvz
Copy link
Contributor

mjbvz commented Nov 9, 2022

I'm closing this since the JS ecosystem has largely moved on writing AMD. This is not an area we plan to invest engineering effort in supporting

@mjbvz mjbvz closed this as completed Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: JavaScript The issue relates to JavaScript specifically In Discussion Not yet reached consensus Suggestion An idea for TypeScript VS Code Tracked There is a VS Code equivalent to this issue
Projects
None yet
Development

No branches or pull requests

10 participants