Skip to content

Move the renderer build into the renderer dir - #1685

Merged
alexcjohnson merged 4 commits into
devfrom
move-renderer-build
Jul 9, 2021
Merged

Move the renderer build into the renderer dir#1685
alexcjohnson merged 4 commits into
devfrom
move-renderer-build

Conversation

@alexcjohnson

@alexcjohnsonalexcjohnson commented Jul 9, 2021

Copy link
Copy Markdown
Collaborator

FYI @HammadTheOne - since the renderer is also an npm package, we want to keep its build output within that package for npm publishing to get all the right files. But I left all the other js deps in the deps folder where you put them in #1611.

@eff-kay please review - this is I think the last change we need before we can publish a new version of Dash.

Contributor Checklist

optionals

  • I have added entry in the CHANGELOG.md

@alexcjohnson
move the renderer build into the renderer dir
416a61c
needed for npm publishing to get all the right files
@alexcjohnson

alexcjohnson commented Jul 9, 2021

Copy link
Copy Markdown
CollaboratorAuthor

The test failure is very strange... async-plotly.js can't be found. It gets added to the <head> tag and executed - the app works just fine - but then somehow the script tag gets deleted before we can see it in the test. I was only able to see it by setting a breakpoint in LazyLoader/plotly.js and then stepping forward a little. I'll have to dig into this tomorrow... perhaps some new webpack behavior?

eff-kay
eff-kay reviewed Jul 9, 2021
View reviewed changes
Comment threaddash/development/build_process.py
eff-kay
eff-kay reviewed Jul 9, 2021
View reviewed changes
Comment threaddash/development/build_process.py
eff-kay
eff-kay reviewed Jul 9, 2021
View reviewed changes
Comment threaddash/development/build_process.py
@eff-kay

eff-kay commented Jul 9, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

some tests fail, but other than that LGTM.

@alexcjohnson

alexcjohnson commented Jul 9, 2021

Copy link
Copy Markdown
CollaboratorAuthor

It looks like webpack 5 indeed removes the script after it executes. AFAICT this was silently introduced with another feature and never documented. I'm just going to tweak the test to ensure the graph rendered and the sync module is present iff expected.

@alexcjohnson
alexcjohnson merged commit 1c0380d into devJul 9, 2021
@alexcjohnson
alexcjohnson deleted the move-renderer-build branch July 9, 2021 20:29
@alexcjohnsonalexcjohnson mentioned this pull request Mar 10, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HammadTheOneHammadTheOneAwaiting requested review from HammadTheOne
@Marc-Andre-RivetMarc-Andre-RivetAwaiting requested review from Marc-Andre-Rivet
1 more reviewer
@eff-kayeff-kayeff-kay left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alexcjohnson@eff-kay