Skip to content

Conversation

@melloc
Copy link
Contributor

@melloc melloc commented Apr 1, 2016

The string 'use strict' on its own line causes part of the program to enter into strict mode, and changes the semantics of some operations (usually making more things throw errors instead of silently failing). This change prevents warning when it's used in a program.

@davepacheco
Copy link
Contributor

Nice. In general, I believe we haven't made changes to the javascriptlint in this repo except for the build system. I had assumed the original project was abandoned, but I see that there have been another 50 subversion commits since this fork. It may be worth trying to sync up with upstream again. Notably, they ripped out Spidermonkey in favor of a pure-JavaScript parser. (See https://sourceforge.net/p/javascriptlint/code/commit_browser; we branched at revision 302.)

@melloc
Copy link
Contributor Author

melloc commented Apr 1, 2016

Ah, cool. I'll take a look into what needs to be done to sync with that repo at some point, and see if I can get this change (or a similar one) accepted upstream.

@davepacheco
Copy link
Contributor

Cool. Definitely not saying we can't land this in the meantime, though if we don't get it upstreamed and we do sync up, we'll probably lose it.

@melloc melloc force-pushed the master branch 2 times, most recently from bd11243 to 08162f2 Compare May 10, 2016 01:19
@melloc
Copy link
Contributor Author

melloc commented May 10, 2016

@davepacheco I've synced up the repo with the latest changes to the subversion repo, as well as two other changes I made: one to not consider placing functions in an array "unusual", and the other to support the "use strict" string.

Matthias Miller added 21 commits May 10, 2016 17:19
…entioned in comment regarding invalid control comments
@davepacheco
Copy link
Contributor

Thanks for doing this. Sorry it's taken me a while to look closely at it. I tried to clone it but ran into this:

$ ./build/install/jsl 
Traceback (most recent call last):
  File "./build/install/jsl", line 13, in <module>
    import javascriptlint
  File "/home/dap/javascriptlint/build/install/javascriptlint/__init__.py", line 2, in <module>
    from jsl import main
  File "/home/dap/javascriptlint/build/install/javascriptlint/jsl.py", line 12, in <module>
    import conf
  File "/home/dap/javascriptlint/build/install/javascriptlint/conf.py", line 7, in <module>
    import version
  File "/home/dap/javascriptlint/build/install/javascriptlint/version.py", line 18, in <module>
    version = '0.5.0/%s' % _getrevnum()
  File "/home/dap/javascriptlint/build/install/javascriptlint/version.py", line 13, in _getrevnum
    raise _BuildError('Error running svnversion: %s' % stderr)
NameError: global name '_BuildError' is not defined

There are a few problems here:

  • my git is too old to support -C, so the command fails
  • _BuildError isn't found; it doesn't appear to be part of the repo anywhere
  • we probably shouldn't have a runtime dependency on the VCS anyway

I think @melloc is already working on this. I'm just updating the ticket to reflect the status.

Once this is resolved, I'll be interested to

  • run the new javascriptlint over a bunch of existing code bases that we check with javascriptlint to see how the output differs
  • make sure the Makefile and README are updated with exactly how to use it
  • make sure it's still easy to incorporate into other repos by submodule
  • make sure we've somehow separated our local changes from the ones upstream. I'm not sure yet what's the best way to do this.
  • announce a new version

@davepacheco
Copy link
Contributor

Thanks for fixing the issue above. On the joyent/manta-marlin repo, where we run it over 54 files, I found two things of note:

  • The new javascriptlint took 24 seconds (compared to 3 seconds for the old one). Definitely sucks, but I'm okay with that, assuming there really is value in keeping sync'd up. (Not having to build binary modules is nice, at least.)
  • The new javascriptlint does not appear to support named function expressions where the function name is used inside the function, like this:
setTimeout(function doSomething() {
        console.log('it works!');
        setTimeout(doSomething, 1000);
}, 1000);

It reports "doSomething" as an undeclared identifier on line 3.

@arekinath
Copy link

Another issue with the new version: the const keyword seems to produce a warning: missing semicolon before statement warning, which is not produced if you just change the const to a var.

@melloc
Copy link
Contributor Author

melloc commented Jun 22, 2016

I've added support for the const keyword, and for introducing the name of a named function expression into the scope of the body of the function. For now, things declared with const are treated as if they are variables. If anyone wants to, it might be a good improvement in the future to warn when assignments are made to things that have been declared const.

I profiled the code with cProfile, and it looks like the majority of the program's time is being spent in the tokenizing code. It's unclear to me how it could be improved without further investigation, but it seems like that is something that can also be investigated in the future.

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