search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] Update Tern & Acorn #10051

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by MarcelGerber Sunday Aug 09, 2015 at 22:16 GMT Originally opened as https://github.com/adobe/brackets/pull/11569

Since Acorn's build process changed (there are no more prebuilt dist files in the repo, but in the npm package), I've switched from having it available as a submodule to just having the files in the file tree. Hope that's fine. The build is based on my fork https://github.com/MarcelGerber/acorn, where I removed a build step that doesn't quite work with Brackets.

Fixes #11568, and hopefully some others, too.

Commit logs: Tern + Release notes Acorn

MarcelGerber included the following code: https://github.com/adobe/brackets/pull/11569/commits

core-ai-bot commented 3 years ago

Comment by MarcelGerber Sunday Aug 09, 2015 at 22:30 GMT

All tests pass now.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Sunday Aug 09, 2015 at 22:38 GMT

Woah. Looks like this upgrade actually finally fixes the problems we had with Ionic/Cordova apps (cc@MiguelCastillo - would be very kind of you if you could test that). That'd be awesome. (At least I cannot repro any longer with an example project where I can repro on master)

core-ai-bot commented 3 years ago

Comment by ficristo Wednesday Aug 12, 2015 at 14:19 GMT

Out of curiosity, can I know why they can't be dependencies in package.json?

core-ai-bot commented 3 years ago

Comment by MarcelGerber Thursday Aug 13, 2015 at 08:48 GMT

The two libraries have to be located in the extension's folder, thus they cannot be part of the root package.json. I don't know for sure why they are not part of a package.json in the thirdparty folder itself, but I guess it's just so you can start "hacking" Brackets in as few steps as possible, where npm install is not needed.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Monday Aug 31, 2015 at 10:04 GMT

I just now updated this to feature the latest Acorn & Tern versions, where Tern now has ES6 support (#11644). If you'd rather have the "normal" Tern upgrade first (it's presumably more stable), let me know and I'll factor the ES6 one out into a new PR.

cc@redmunds@nethip for review. Would like to get this in fast so it has a long testing phase on master.

core-ai-bot commented 3 years ago

Comment by MiguelCastillo Wednesday Sep 02, 2015 at 15:12 GMT

@MarcelGerber - tern 0.15 is just released a few hours ago. I think that it will be worth doing that version since ES6 has been more thoroughly tested and bugs fixed. That should definitely improve things.

Some things to keep in mind is that there is a new plugin called module, which needs to be enabled in order to es6 to work well. For example, this is what my tern config looks like now.

{
    "libs": [
        "browser",
        "chai",
        "ecma5",
        "ecma6",
        "browser",
        "jquery"
    ],
    "loadEagerly": [],
    "async": true,
    "plugins": {
        "doc_comment": {},
        "requirejs": {},
        "es_modules": {},
        "modules": {}
    }
}
core-ai-bot commented 3 years ago

Comment by MarcelGerber Wednesday Sep 02, 2015 at 16:08 GMT

Thanks@MiguelCastillo Updating Tern was already on my agenda, and I just committed and pushed that upgrade.

I'll try to look at the modules plugin after my 4-day-trip to Berlin, on Sunday/Monday. Or if you feel like it, feel free to do this yourself :)

core-ai-bot commented 3 years ago

Comment by abose Friday Sep 11, 2015 at 13:59 GMT

As this is a core change, Moving to next release as all external integrations happen at the beginning of the cycle. @MarcelGerber Will target this immediately after rel 1.5 is out.

core-ai-bot commented 3 years ago

Comment by ingorichter Monday Sep 14, 2015 at 18:53 GMT

@abose what is the timeline for the 1.5 release? I'm just asking, since it's not clear to me, if this means we are holding off PR's for days, weeks or months.

core-ai-bot commented 3 years ago

Comment by abose Tuesday Sep 15, 2015 at 09:11 GMT

1.5 is due within the next 2 weeks and the move to release branch is by next week. So the holdoff is probably till next week :)

core-ai-bot commented 3 years ago

Comment by ingorichter Wednesday Sep 16, 2015 at 05:06 GMT

That's great@abose! Then let's hold it off until then.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Saturday Jan 30, 2016 at 22:51 GMT

Updated once again to include the latest & greatest.

core-ai-bot commented 3 years ago

Comment by busykai Friday Feb 19, 2016 at 00:38 GMT

@MarcelGerber, accidentally tried more or less the same and hit the unfortunate change in acorn as well. I think replacing it with a copy is the right thing to do, except these doubts:

  1. why do we need all the rest of the files in thirdparty/acorn? perhaps we only need to keep the LICENSE.md, remove the rest except dist/*, and move all the files from dist/ to the root (or leave it there if tern relies on them being in dist).
  2. we need to be explicit in the comments which version/commit id of acorn is being committed. since it's not a submodule any longer, it's needed for the bookkeeping.
core-ai-bot commented 3 years ago

Comment by MarcelGerber Friday Feb 19, 2016 at 22:24 GMT

@busykai Where'd you suggest putting the commit SHA (we already have the version embedded in code)? Otherwise, we could also simply settle with release versions, so we can rely on the version number by itself, not having to worry about the SHA.

core-ai-bot commented 3 years ago

Comment by busykai Friday Feb 19, 2016 at 23:21 GMT

@MarcelGerber, I was thinking additionally a version tag/sha in the commit comment. So that you could see with git log the history of the versions.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Friday Feb 19, 2016 at 23:23 GMT

Ah yes, that makes sense. But considering I'll rebase these commits away anyhow at some point, we can do that later on. For now, I'd like to see whether this actually works as expected.

core-ai-bot commented 3 years ago

Comment by busykai Saturday Feb 20, 2016 at 00:36 GMT

@MarcelGerber, understood. What about removing all the acorn junk? We don't need it there (provided we know where to take it from): since it's no longer a submodule, having it in thirdparty/acorn does not make any sense.

I'll take a look at it functionally and run some tests this weekend.@MiguelCastillo also was going to get back to this.

core-ai-bot commented 3 years ago

Comment by MiguelCastillo Saturday Feb 20, 2016 at 14:29 GMT

@MarcelGerber@busykai Ok, I had to read the whole thing again. :)

I'd really prefer using npm packages. It just makes our lives easier because we don't need to worry maintaining yet another repo.

One thing that I think we can do as a separate PR is to move thirdparty deps into the package.json as way to manage dependency upgrades. And we can manage copying these modules from node_modules into thirdparty (or any other place) in our gruntfile.js, which we already do for a few other things. I can look into that separately. But for now, adding tern as a dependency and adding a config in the gruntfile.js to copy the needed files to the right place will be a more straight forward workflow for future upgrades. I see that there are concerns with setting up brackets for hacking. I am not too concerned about that because we will be checking in the dependencies into the thirdparty directory anyways. Upgrades will continue to be a manual process of running npm install and putting out a PR to merge the new stuff into the correct place. No more git submodule init -u ... -whtver please :D

I am going to pull this down right now and take it out for test.

core-ai-bot commented 3 years ago

Comment by ficristo Saturday Feb 20, 2016 at 14:36 GMT

@MiguelCastillo you should take a look at https://github.com/adobe/brackets/issues/12006

core-ai-bot commented 3 years ago

Comment by MiguelCastillo Saturday Feb 20, 2016 at 14:38 GMT

@ficristo Well thank you very much sir!

core-ai-bot commented 3 years ago

Comment by petetnt Monday Feb 22, 2016 at 08:11 GMT

RE: the requirement for the files being under /dist

You can map the paths in https://github.com/adobe/brackets/blob/master/src/main.js#L31 to your liking; you just need to make sure that acorn_loose which depends on acorn (iirc) can find it.

See the react example that got merged some time last week https://github.com/petetnt/brackets/blob/71d1442c083d48d671ac3905ed70ee2ac37bebe3/src/main.js#L31 (ReactDOM requires react).

core-ai-bot commented 3 years ago

Comment by MarcelGerber Sunday May 15, 2016 at 15:08 GMT

@MiguelCastillo@busykai@abose I've once again updated to the latest versions of both Acorn and Tern. I haven't done much testing, but it seems to at least work.

Also, I've stripped out most of the Acorn files, so we're only left with the dist folder plus AUTHORS, README and LICENSE files.

Would love if you could take this for a test (and I will too!)

core-ai-bot commented 3 years ago

Comment by MiguelCastillo Sunday May 15, 2016 at 15:23 GMT

Ok great! I am pulling it right now.

core-ai-bot commented 3 years ago

Comment by MiguelCastillo Sunday May 15, 2016 at 15:38 GMT

@MarcelGerber help me understand where we stand with this PR. We are continuing to use tern as a submodule and we have pulled out acorn to be from npm?

core-ai-bot commented 3 years ago

Comment by MarcelGerber Sunday May 15, 2016 at 15:56 GMT

Tern is still a submodule, that's true.

Acorn is based of a build of my fork (https://github.com/MarcelGerber/acorn). We cannot use a submodule anymore because over in the repo, only source files are available, no built ones. But we also cannot use the npm version because there's some strange custom require logic going on (apparently to enable usage with Browserify), which I couldn't get to work with our code (@busykai didn't either). So I went with a custom build based off a fork for now (which has the custom require removed: https://github.com/MarcelGerber/acorn/commit/77e0fc99d47bcc4aff8e978a291fa3d9de635a1c). Not the best solution, I know that.

Also, I've just pushed a new commit to workaround this issue: https://github.com/ternjs/acorn/issues/412 Quite surprisingly, frankly said, most unit tests pass now :)

core-ai-bot commented 3 years ago

Comment by ficristo Wednesday Jul 20, 2016 at 18:13 GMT

IMHO we should land https://github.com/adobe/brackets/pull/11948 and then use the node counterpart.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Wednesday Aug 17, 2016 at 22:53 GMT

Updated again. Works fine and all tests pass (that is by no means to suggest a merge! :shipit: 🔫 ).

As Acorn updated their build process, we... sadly still cannot use its output as is. So I applied another patch, which you can see here: https://github.com/MarcelGerber/acorn/tree/brackets-fix

core-ai-bot commented 3 years ago

Comment by ficristo Thursday Aug 18, 2016 at 06:06 GMT

I really want this for next release, I only think #11948 is a better approach in the long term.

core-ai-bot commented 3 years ago

Comment by zaggino Thursday Aug 18, 2016 at 06:17 GMT

Guys, can we please stick to npm dependencies or submodules and not push heaps of thirdparty code to the repository itself?

core-ai-bot commented 3 years ago

Comment by MarcelGerber Thursday Aug 18, 2016 at 10:04 GMT

@ficristo@zaggino I just wanted to get it to the latest version so we don't get lost in too old a version. If we can land both of these other PRs before this one, that's all fine by me. This is more or less a proof-of-work, and a proof that it still works :)