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

webpack-dash-dynamic-import: Don't add JS to chunks created by mini-css-extract-plugin #1855

Closed

Conversation

ryanand26
Copy link
Contributor

While using the dash-component-boilerplate I was trying to extract my CSS in a separate file but was facing issues as this plugin adds additional content.

The plugin assumes that all chunks are JS chunks but when using mini-css-extract-plugin this isn't the case.

Fortunately mini-css-extract-plugin always names it's chunks the same so this if allowed me to avoid adding that content for this case. There are more involved ways to check the type of the import though so no worries if this doesn't pass muster with you all.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Add if condition to check where mini-css-extract-plugin is the chunk name and skip adding content
    • Increment package.json version
  • [] I have run the tests locally and they passed. (refer to testing section in contributing)
    • [] Are there existing tests for this?

@anders-kiaer
Copy link
Contributor

Nice one @ryanand26. This appears to be what stopped me in plotly/dash-core-components#753 (comment) a couple of years ago when I did a quick draft PR on separating CSS on production build for dcc in plotly/dash-core-components#753.

Solving the small issue addressed in this PR is a step towards also solving plotly/dash-core-components#752.

@alexcjohnson
Copy link
Collaborator

Thanks @ryanand26 - this looks great.

The test failures worry me, they don't seem related to your changes but if you're up for investigating please do - that's clearly a much bigger scope than the intent of this PR, it's just that it may take some time before Plotly folks find time to dig in.

The ones I'm specifically worried about are these ones below - as if the latest Chrome/Chromedriver updates broke something, or something changed in Selenium? 😱 We can certainly try just running the tests again (ie give it an empty commit - unfortunately just rerunning on CircleCI doesn't work well in this project) but these particular ones look likely to be repeatable:

markdown_options = None, new_tab = True, cell_selectable = True
    @pytest.mark.parametrize(
        "markdown_options,new_tab",
        [
            [None, True],
            [dict(linkTarget="_blank"), True],
            [dict(linkTarget="_self"), False],
        ],
    )
    @pytest.mark.parametrize("cell_selectable", [True, False])
    def test_tmdl001_click_markdown_link(test, markdown_options, new_tab, cell_selectable):
        test.start_server(get_app(cell_selectable, markdown_options))
        target = test.table("table")
        assert len(test.driver.window_handles) == 1
        target.cell(0, "a").get().find_element_by_css_selector("a").click()
        # Make sure the new tab is what's expected
        if new_tab:
            assert target.cell(0, "a").is_selected() == cell_selectable
            assert len(test.driver.window_handles) == 2
>           test.driver.switch_to_window(test.driver.window_handles[1])
E           AttributeError: 'WebDriver' object has no attribute 'switch_to_window'
tests/selenium/test_markdown_link.py:53: AttributeError

@ryanand26
Copy link
Contributor Author

Hey Alex, I pushed another commit but looks like it's getting the same failures. It's been a long long time since I had to debug selenium so not really the right person to help out with that.

No rush from me on having this published. As it's only a little block of code I can have in the local repo and imported into webpack.config.js from there until Plotly take a look.

@alexcjohnson alexcjohnson mentioned this pull request Dec 8, 2021
2 tasks
@alexcjohnson
Copy link
Collaborator

Closing in favor of #1858

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.

3 participants