Add build fixups + use Artifacts #13

Felix-Gauthier · 2021-12-09T23:42:10Z

This is following 2 other closed PRs (#10 and #12) made I was getting familiar working with a "Dash component project", so to speak.

Third time's the charm!

Resolves #11

To-do:

update url in Artifacts.toml with value from [POC] Use Julia artifacts for .js deps (for use with PackageCompiler) #12 (comment)

Felix-Gauthier · 2021-12-09T23:43:44Z

dash_textarea_autocomplete/package-info.json

+    "build:create-tar-file-from-deps": "tar -czf ./deps.tar.gz ./deps && mv ./deps.tar.gz ./deps",
+    "build": "npm run build:js && npm run build:backends && npm run build:create-tar-file-from-deps",
+    "build:activated": "npm run build:js && npm run build:backends-activated",
+    "postbuild:fixup-external-url-jl": "sed -i 's/https:\\/\\/unpkg.com\\/dash_textarea_autocomplete/https:\\/\\/unpkg.com\\/dash-textarea-autocomplete/' src/DashTextareaAutocomplete.jl",
+    "postbuild:fixup-dash-version-project-jl": "sed -i 's/Dash = \"0.1.3\"/Dash = \"0.1.3, 1.0\"/' Project.toml",
+    "postbuild:fixup-resources-path-jl": "sed -i 's/realpath(joinpath( @__DIR__, \"..\", \"deps\"))/artifact\"dash_textarea_autocomplete_resources\"/' src/DashTextareaAutocomplete.jl",
+    "postbuild": "npm run postbuild:fixup-external-url-jl && npm run postbuild:fixup-dash-version-project-jl && npm run postbuild:fixup-resources-path-jl"


This is a lot of manual fix-ups, it would be nice to follow up on creating PR in the dash repo so we can remove each of these one by one. We can create Issues here to track it potentially ?

Good point!

I would start by opening an issue in https://github.com/plotly/dash/issues mentioning that the "proper" way (and the one that's work with PackageCompiler.jl 😏 ) to handle artifacts in Julia now is by defining Artifact.toml.

Then, we should mention that Artifact.toml only accepts tarball, so we would need to tar the min.js and min.js.map files together in the build process so that the tarball is part of the npm registry and has a permanent URL available from unpkg.com

We could also reference this PR (they can see it, it's opensource 🎊 ) to put things in context.

- Update Julia compat bound to 1.3 to use Pkg.Artifacts

package.json

dash_textarea_autocomplete/package-info.json

etpinard · 2021-12-10T14:57:11Z

src/DashTextareaAutocomplete.jl

@@ -16,14 +17,14 @@ function __init__()
            [
                DashBase.Resource(
    relative_package_path = "dash_textarea_autocomplete.min.js",
-    external_url = "https://unpkg.com/dash_textarea_autocomplete@1.1.0/dash_textarea_autocomplete/dash_textarea_autocomplete.min.js",
+    external_url = "https://unpkg.com/dash-textarea-autocomplete@1.1.0/dash_textarea_autocomplete/dash_textarea_autocomplete.min.js",


Do we actually need these externarl_url definitions now that we're set to use Artifacts and a tarball?

Well, we didn't need them before since they never actually worked 😏

If we're going to have an external_url resource, it'd be nice if it was a functioning link even if we won't rely on it because of artifacts.

Maybe there's a case where artifacts can't be acquired and this would be the fallback ?

Felix-Gauthier · 2021-12-10T21:04:04Z

Ok so a lot of changes in the last 6 commits, mostly about adding the url value and moving the postbuild stuff to a separate file, as well as committing the deps tarball to the repo.

I think this should be good for final review pass and version bump, but we have to be extra sure the url should work before merging & tagging : it's not dramatic if we have to do a v1.2.1 to correct it but it would be nice to get it right on the first try.

postbuild_fixups.sh

etpinard · 2021-12-10T21:39:07Z

Project.toml

@@ -1,12 +1,13 @@
+


Hmm, that's new. Something must be off in the sed command. Can you try to modify it so that we don't get this spurious newline.

Ha, nevermind.

Looks like the current main version

https://github.com/etpinard/dash-textarea-autocomplete/blob/main/Project.toml#L1

has a newline at the top of file also.

yeah that would be part of a PR to https://github.com/plotly/dash/blob/dev/dash/development/_jl_components_generation.py

looking at it, it seems like https://github.com/plotly/dash/blob/dev/dash/development/_jl_components_generation.py#L87 is part of main , so why aren't we getting that with npm run build ? 🤔

looking at it, it seems like https://github.com/plotly/dash/blob/dev/dash/development/_jl_components_generation.py#L87 is part of main , so why aren't we getting that with npm run build ? thinking

That's their dev branch, so yeah it will be part of the next dash.py release.

oooh -- right, sorry.

etpinard · 2021-12-10T21:45:16Z

CONTRIBUTING.md

+        **N.B.**
+        + We use `postbuild_fixups.sh` to re-write certain lines in the generated Julia files of this package to allow usage of Julia's [Artifact](https://pkgdocs.julialang.org/v1/artifacts/#Artifacts) system.


This is fine, we do need to run npm run postbuild while developing the package.

But more importantly, we should also mention npm run postbuild in the Create a production build and publish section below.

From what I've been doing, npm run build seems to run every step of the "scripts" section in package.json, regardless of the name as long as the script entry starts with npm run ...

I've haven't had to do npm run postbuild yet to have the postbuild:<> step run so far.

Maybe it's different for a production build though ?

Ah I see. Looks like the pre and post complementary scripts work even for user-defined scripts like build.

https://docs.npmjs.com/cli/v6/using-npm/scripts

https://thomasxbanks.com/articles/pre-post-npm-scripts/

Is this a new feature or just something I forgotten from my JS days 🤔

etpinard · 2021-12-10T21:52:51Z

Awesome. This looks great! After adding one line in the CONTRIBUTING.md file, I'd be happy with giving this a go!

I think this should be good for final review pass and version bump, but we have to be extra sure the url should work before merging & tagging : it's not dramatic if we have to do a v1.2.1 to correct it but it would be nice to get it right on the first try.

Right, I don't mind giving this v1.2.0 a shot and quickly make a fix in v1.2.1 if things don't work. Let's be honest, this project isn't that popular (yet 🙃 )

That said, releasing this thing isn't that simple. Make sure you review

https://github.com/etpinard/dash-textarea-autocomplete/blob/main/CONTRIBUTING.md#create-a-production-build-and-publish

namely, you'll need an https://www.npmjs.com/ account (to publish the javascript package), https://pypi.org/ account (to publish the python package) and then I'll need to add you as a contributor to both registries.

etpinard · 2021-12-10T21:54:20Z

Right, I don't mind giving this v1.2.0 a shot and quickly make a fix in v1.2.1

An important corollary here: let's not try do this on a Friday afternoon. Mondays are much better for these sort of things 😄

etpinard

Great. Let's give this a shot!

@Felix-Gauthier go ahead and merge this PR to main and follow the CONTRIBUTING.md to make the release from main.

Felix-Gauthier added 3 commits December 9, 2021 23:29

Add 'fixup' and 'create tar file' steps to build

093c15a

New Artifacts.toml

f9b3e08

Updated resources_path and external_url

4d8f0da

Felix-Gauthier commented Dec 9, 2021

View reviewed changes

Felix-Gauthier and others added 2 commits December 9, 2021 23:55

- Add Pkg as direct dep

58e44e5

- Update Julia compat bound to 1.3 to use Pkg.Artifacts

Add using Artifacts to module

75790fe

Felix-Gauthier mentioned this pull request Dec 9, 2021

[POC] Use Julia artifacts for .js deps (for use with PackageCompiler) #12

Closed

Add tarball sha values

b4e89dd

etpinard reviewed Dec 10, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

etpinard reviewed Dec 10, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

etpinard reviewed Dec 10, 2021

View reviewed changes

dash_textarea_autocomplete/package-info.json Show resolved Hide resolved

etpinard reviewed Dec 10, 2021

View reviewed changes

Felix-Gauthier added 6 commits December 10, 2021 20:54

Add url

a6904cd

Specify linux-only builds

751dbc9

Add deps tarball

6fddbfe

New postbuild_fixups.sh

53ac134

Update postbuild:fixups build step

025e861

Update after build script changes

f6ede40

Felix-Gauthier marked this pull request as ready for review December 10, 2021 21:04

Felix-Gauthier commented Dec 10, 2021

View reviewed changes

postbuild_fixups.sh Show resolved Hide resolved

etpinard reviewed Dec 10, 2021

View reviewed changes

etpinard approved these changes Dec 13, 2021

View reviewed changes

Felix-Gauthier merged commit 3f110cd into main Dec 13, 2021

Felix-Gauthier deleted the add-build-fixups-and-add-artifacts branch December 13, 2021 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build fixups + use Artifacts #13

Add build fixups + use Artifacts #13

Felix-Gauthier commented Dec 9, 2021 •

edited

Loading

Felix-Gauthier Dec 9, 2021

etpinard Dec 10, 2021

etpinard Dec 10, 2021

Felix-Gauthier Dec 10, 2021

Felix-Gauthier commented Dec 10, 2021

etpinard Dec 10, 2021

etpinard Dec 10, 2021

Felix-Gauthier Dec 10, 2021

etpinard Dec 10, 2021

Felix-Gauthier Dec 10, 2021

etpinard Dec 10, 2021

Felix-Gauthier Dec 10, 2021

etpinard Dec 13, 2021 •

edited

Loading

etpinard commented Dec 10, 2021 •

edited

Loading

etpinard commented Dec 10, 2021

etpinard left a comment

		N.B.
		+ We use `postbuild_fixups.sh` to re-write certain lines in the generated Julia files of this package to allow usage of Julia's [Artifact](https://pkgdocs.julialang.org/v1/artifacts/#Artifacts) system.

Add build fixups + use Artifacts #13

Add build fixups + use Artifacts #13

Conversation

Felix-Gauthier commented Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felix-Gauthier commented Dec 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

etpinard commented Dec 10, 2021 • edited Loading

etpinard commented Dec 10, 2021

etpinard left a comment

Choose a reason for hiding this comment

Felix-Gauthier commented Dec 9, 2021 •

edited

Loading

etpinard Dec 13, 2021 •

edited

Loading

etpinard commented Dec 10, 2021 •

edited

Loading