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

Updating LinkedUp to work with 0.6.2 #39

Merged
merged 8 commits into from
Aug 11, 2020
Merged

Updating LinkedUp to work with 0.6.2 #39

merged 8 commits into from
Aug 11, 2020

Conversation

alexabsmith
Copy link
Contributor

No description provided.

@alexabsmith alexabsmith requested review from andrewwylde, lsgunnlsgunn and a user August 6, 2020 20:16
@alexabsmith alexabsmith requested a review from a team as a code owner August 6, 2020 20:16
Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com>
@@ -52,15 +48,7 @@ function generateWebpackConfigForCanister(name, info) {
filename: "index.js",
path: path.join(outputRoot, "assets"),
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I did something wrong, but I got this error when I tried opening the entrypoint in a browser (I did at least clear my cache):

An error happened:
http://localhost:8000/bootstrap.js:2:43236
asyncFunctionResume@[native code]
[native code]
promiseReactionJobWithoutPromise@[native code]
promiseReactionJob@[native code]

There are some more differences in the webpack.config.js for new projects that I don't see reflected here (though I don't know if they are required):

      "source": [
        "src/linkedup_assets/assets",
        "dist/linkedup_assets/"
      ],

Copy link
Contributor

@lsgunnlsgunn lsgunnlsgunn Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I got the same error when pulling from this update. Interestingly, it worked without the change (to the assets directory) for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it worked for me too when we had "canisters/linkedup_assets/assets" included..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. What's happening is webpack.config.js is writing its output into canisters/linkedup_assets/assets.

So it will work to put canisters/linkedup_assets/assets back into sources, because that's where webpack is putting the files, but that means dfx will use canisters/linkedup_assets/assets as both a source and as a destination when "building" the assets. That might cause problems the second or third time a build is run (or it might "just work").

My understanding of how newly-created projects manage this:

ericswanson-dfinity and others added 4 commits August 7, 2020 13:24
Copy assets from dist directory.
Update readme with new dfx command steps.
* Refresh content for the LinkedUp readme

* Fix typo in URL

* Remove ADOC notation

* Change dfx.json

* fix travis.yml file

* fix bootstrap.sh file

* change path in bootstrap.sh file

* Update .travis.yml

Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com>

* fix canister id

* delete old encoding line

* remove uneeded id line

* remove ID line

* delete unused crc8 python script

Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com>
Co-authored-by: Prithvi Shahi <50885601+p-shahi@users.noreply.github.com>
Co-authored-by: Prithvi Shahi <prithvi@dfinity.org>
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good once those two unused fields are removed from dfx.json

@andrewwylde
Copy link
Contributor

kk, I fixed the unused properties.

Copy link
Contributor

@andrewwylde andrewwylde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't wanna bulldoze, but I approve this PR!

@andrewwylde andrewwylde merged commit 84b1b57 into master Aug 11, 2020
@andrewwylde andrewwylde deleted the as/v0.6.2 branch August 11, 2020 18:31
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.

4 participants