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.

Sign up for GitHub

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

Jump to bottom

[CLOSED] Update Tern & Acorn #10051

Open
core-ai-bot opened this issue Aug 30, 2021 · 30 comments
Open

[CLOSED] Update Tern & Acorn #10051

core-ai-bot opened this issue Aug 30, 2021 · 30 comments

Comments

@core-ai-bot
Copy link
Member

Issue by MarcelGerber
Sunday Aug 09, 2015 at 22:16 GMT
Originally opened as adobe/brackets#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
Copy link
Member Author

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

All tests pass now.

@core-ai-bot
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

Updated once again to include the latest & greatest.

@core-ai-bot
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@MiguelCastillo you should take a look at adobe/brackets#12006

@core-ai-bot
Copy link
Member Author

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

@ficristo Well thank you very much sir!

@core-ai-bot
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

Ok great! I am pulling it right now.

@core-ai-bot
Copy link
Member Author

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
Copy link
Member Author

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: marcelgerber/acorn@77e0fc9). Not the best solution, I know that.

Also, I've just pushed a new commit to workaround this issue: acornjs/acorn#412
Quite surprisingly, frankly said, most unit tests pass now :)

@core-ai-bot
Copy link
Member Author

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

IMHO we should land adobe/brackets#11948 and then use the node counterpart.

@core-ai-bot
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot