Skip to content

Support asconfig.json#1238

Merged
dcodeIO merged 69 commits into
AssemblyScript:masterfrom
jtenner:asconfig
Jul 16, 2020
Merged

Support asconfig.json#1238
dcodeIO merged 69 commits into
AssemblyScript:masterfrom
jtenner:asconfig

Conversation

@jtenner

@jtenner jtenner commented Apr 22, 2020

Copy link
Copy Markdown
Contributor

I gave this change a lot of thought before I even started coding it, but I bet there are multiple ways to improve this pull request.

  • I've read the contributing guidelines

Todo:

  • Utilize new merge mechanic
  • Support asconfig.include? (string[])
  • Testing (still more that needs to be done
  • Asinit changes
    • add default asconfig
    • modify existing asconfig

Comment thread cli/asconfig.json Outdated
Comment thread cli/asconfig.js Outdated
Comment thread cli/asc.js Outdated
@dcodeIO

dcodeIO commented Apr 23, 2020

Copy link
Copy Markdown
Member

Regarding the algorithm, I'm wondering if this can be integrated into options.parse by adding one more argument, an object of previously parsed options. Now, if the object is given, it simply wouldn't overwrite existing options with defaults? So, one starts with an empty object {} that will be fully populated with either given options or defaults since there are no existing ones on it, and get a fully populated object. That object can now be given to parse again, but this time, since all keys exist, would not apply defaults again and only overwrite with given options?

Edit: Oh, wait, this operates on args, not opts. But perhaps there is a way to put all of this into util/options.js, and let it reuse the relevant parts?

Comment thread cli/asconfig.js Outdated
@jtenner

jtenner commented May 31, 2020

Copy link
Copy Markdown
Contributor Author

It looks like enabled and disabled logic is more complicated than I thought. Because the cli provides a string array, it needs to be treated as a set of immutable flags by the time it gets passed to resolve the asconfig.

What's more difficult is that each nested asconfig level needs to handle enable and disable flags before they get overridden by the cli.

I am proposing that the enable and disable flags are removed from the MVP.

@jtenner jtenner changed the title Support asconfig.json Support mvp asconfig.json May 31, 2020
@jtenner jtenner changed the title Support mvp asconfig.json Support asconfig.json Jun 17, 2020
@jtenner

jtenner commented Jun 17, 2020

Copy link
Copy Markdown
Contributor Author

Restarting my efforts here. Need to think this through a little bit more.

Comment thread cli/asc.js Outdated
@jtenner

jtenner commented Jun 20, 2020

Copy link
Copy Markdown
Contributor Author

Progress:

  • Added support for the ability of asconfig to specify entry points
    • This causes the compiler to add entry points from extended configurations too
    • Duplicate entry points are calculated and removed before they are passed to the compiler
    • Entry points must be relative to the asconfig, (e.g. ./asconfig.json can point to ./assembly/index.ts, ignoring any provided basedir)

We still need to add some testing projects in the repo to verify this functionality works. This pull request is becoming much larger than I previously thought.

@jtenner

jtenner commented Jun 20, 2020

Copy link
Copy Markdown
Contributor Author

@dcodeIO If perhaps someone would like to extend multiple configs to obtain their entry points, this becomes impossible because extends is currently defined as a string. Perhaps this field is best designed as a string[].

Example, I have a library that would like to obtain lib entries from two distinct projects:

{
  "targets": {
    "release": {
       "optimizeLevel": 3,
       "shrinkLevel": 2
     }
  },
  "extends": [
    "./node_modules/@assemblyscript/node/asconfig.json",
    "./node_modules/some-utf8-lib/asconfig.json"
  ]
}

Now both sets of lib entries are added to the compilation.

Thoughts?

@dcodeIO

dcodeIO commented Jun 20, 2020

Copy link
Copy Markdown
Member

Looks like this is getting dangerously close to substituting what imports would do, but given that the same isn't achievable with just imports when it comes to additional entries or similar, I'm not necessarily opposed to allow extends to be an array as an option, as long as it can also be a string which I'd assume to be the more common case?

@jtenner

jtenner commented Jun 20, 2020

Copy link
Copy Markdown
Contributor Author

Why not both? Sounds good to me.

@jtenner

jtenner commented Jun 30, 2020

Copy link
Copy Markdown
Contributor Author

I've switched to supporting an MVP in this pull request again. Just taking a bit of time to figure out how to do testing. I suspect I'll be ready for review in a week.

Comment thread cli/asc.js Outdated
@dcodeIO

dcodeIO commented Jul 15, 2020

Copy link
Copy Markdown
Member

Problem with the wat file seems to be here. Moving that below the args.outFile check so whateverFile is populated should fix it.

@jtenner

jtenner commented Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

Problem with the wat file seems to be here. Moving that below the args.outFile check so whateverFile is populated should fix it.

I'm sorry, what do you mean by "that?"

@dcodeIO

dcodeIO commented Jul 15, 2020

Copy link
Copy Markdown
Member

I mean the linked declaration of hasOutput. If its value is set after deriving the format of outFile, the behavior will be correct. If you think that should be a separate PR, this PR here can be fixed by using --binaryFile instead of --outFile. Or you can just fix the cause here if you prefer. In general this PR looks pretty good now with the last comments addressed :)

@jtenner

jtenner commented Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

Oh I see. I thought this was my fault 😅 . Yeah. Let's pull that out into a separate PR. Just wondering if everything here is appropriately addressed now?

@dcodeIO

dcodeIO commented Jul 15, 2020

Copy link
Copy Markdown
Member

Merging master should fix the wat problem.

@jtenner

jtenner commented Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

Please let me know if I can help finish this. I am very excited to refactor my cli completely to account for asconfig.

@dcodeIO

dcodeIO commented Jul 15, 2020

Copy link
Copy Markdown
Member

Looks good to merge once CI passed, unless there are remaining objections?

@jtenner

jtenner commented Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

I wish I could extend multiple configurations, but I really like the elegance of this algorithm using a while loop like I designed it more. Thank you for your feedback!

@jtenner

jtenner commented Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

Looks like it's all set.

@dcodeIO dcodeIO merged commit 5207bcb into AssemblyScript:master Jul 16, 2020
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 0.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dcodeIO

dcodeIO commented Jul 19, 2020

Copy link
Copy Markdown
Member

Turned out that path.relative does all sorts of funny things in the browser, like stripping away the first character of entry files, but only in bundles and otherwise hard to reproduce. Not particular a fault of this PR, yet browser builds are still heavily searching for that ominous odule.ts instead of functioning properly.

@dcodeIO dcodeIO mentioned this pull request Jul 19, 2020
@jtenner

jtenner commented Jul 19, 2020

Copy link
Copy Markdown
Contributor Author

Right. Should we add a test to make sure browserified bundles work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants