-
Notifications
You must be signed in to change notification settings - Fork 634
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
[DYN-2316] feature: consume librarie.js as npm package #15228
[DYN-2316] feature: consume librarie.js as npm package #15228
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-2316
@@ -0,0 +1,45 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this file needs to be hard copied over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can removed it.
UI Smoke TestsTest: success. 2 passed, 0 failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some minor comments and some comments about the files included in this PR.
</PropertyGroup> | ||
|
||
<!--Extracts the file to /package--> | ||
<Exec Command="tar -xzf $(MSBuildProjectDirectory)\$(Last).tgz --strip-components=1 --directory=web/library"></Exec> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,45 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should NOT include this files as part of your PR:
- librarie.min.js
- librarie.min.js.LICENSE.txt
- librarie.min.js.map
it's is supposed that when the LibraryViewExtensionWebView2 project is build it will be downloading the package, unzip it ...... then this files will be present at that time, am i right?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen librarie.min.js needs to be included (I suppose as it is an EmbeddedResource ) otherwise we'll get an error in Library controller related to StreamReader
. @RobertGlobant20 do you know why this might be happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Enzo707 have you resolved this or do you still need unblocking here?
@@ -287,23 +287,23 @@ private string ReplaceUrlWithBase64Image(string html, string minifiedURL, bool m | |||
//list of resources which have paths embedded directly into the source. | |||
private readonly Tuple<string, bool>[] dynamicResourcePaths = new Tuple<string, bool>[] | |||
{ | |||
Tuple.Create("/resources/ArtifaktElement-Bold.woff",true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the "/" is not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the "/" is not needed?
Because we changed the public path in package so the bundle has changed
src/LibraryViewExtensionWebView2/Packages/LibrarieJS/package.json
Outdated
Show resolved
Hide resolved
Why are you committing any of the npm package contents into the repo? Ah, I see you and @RobertGlobant20 discussed a bit above - you should be able to make sure that the build pulls the npm package before it attempts to embed the resource. I think that having a version of the min file committed and an npm package pulled will lead to confusing build results where the actual embedded version is unclear - that is exactly what we are trying to avoid by pulling a specific version of the package. |
Make sense, I've removed all hard copied files that will be installed by npm package. |
what's with the failing self serve job? |
It timed out, I restarted it |
@QilongTang I've made a rebase master on this branch and all tests passed |
…moDS#15225) Co-authored-by: Aaron (Qilong) <[email protected]>
Co-authored-by: Trygve Wastvedt <[email protected]> Co-authored-by: pinzart90 <[email protected]>
Co-authored-by: Trygve Wastvedt <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]>
Co-authored-by: Trygve Wastvedt <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]>
@@ -1,23 +1,26 @@ | |||
## In Depth | |||
|
|||
The Define Data node validates the data type of incoming data. It can be used to ensure local data is of the desired type and is also designed to be used as an input or output node, declaring the type of data a graph expects or provides. The node supports a selection of commonly used Dynamo data types, for example 'String', 'Point', or 'Boolean'. The full list of supported data types is available in the drop-down menu of the node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like your PR is now touching unrelated files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is this just showing commits that are already in master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjkkirschner yep, those commits are in master already
@Enzo707 Does not feel rebase would be the best option here, next time maybe just merge the changes on master to your fork? |
@QilongTang Got it! |
Purpose
This PR implements librarie.js consuming as npm package
Ref.: DYN-2316
Declarations
Check these if you believe they are true
*.resx
filesReviewers
@QilongTang
@RobertGlobant20
FYIs
@mjkkirschner