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

Build: Fix copying top-level demos files #216

Merged
merged 2 commits into from
Jul 30, 2024
Merged

Conversation

mgol
Copy link
Member

@mgol mgol commented Jul 29, 2024

The logic to copy files from demos was faulty for top-level files - it was
copying them to the undefined subdirectory. This is now fixed.

Also, dot-files are no longer copied; we don't need the ESLint config copied,
for example.

Also, the first commit contains some minor cleanup:

  1. Remove the externalDir variable that was computed but not used.
  2. Remove the local unused globalize variable.
  3. Declare the local external variable - previously, build code was
    overwriting the external global.

mgol added 2 commits July 29, 2024 21:57
1. Remove the `externalDir` variable that was computed but not used.
2. Remove the local unused `globalize` variable.
3. Declare the local `external` variable - previously, build code was
   overwriting the `external` global.
The logic to copy files from demos was faulty for top-level files - it was
copying them to the `undefined` subdirectory. This is now fixed.

Also, dot-files are no longer copied; we don't need the ESLint config copied,
for example.
@mgol mgol requested review from Krinkle and timmywil July 29, 2024 19:59
@mgol
Copy link
Member Author

mgol commented Jul 29, 2024

I cannot fully test this locally as demo embeds don't load in my local setup based on jquery-wp-docker but grunt deploy worked and prior to this fix a few files like search.js were copied to the undefined directory.

You can also verify that while https://jqueryui.com/resources/demos/search.js is a 404, https://jqueryui.com/resources/demos/undefined/search.js does exist. 🙂

I think we can merge it even without fully testing locally.

@mgol
Copy link
Member Author

mgol commented Jul 29, 2024

@Krinkle one thing - while this PR definitely fixes some copying issues, I don't think it will fix the demos in itself. jQuery UI deploy code deAMDifies JS the HTML and the resulting HTML doesn't use RequireJS at all. search.js still does, though, as only the HTML files are rewritten like that...

@mgol
Copy link
Member Author

mgol commented Jul 29, 2024

The proper fix will most likely also require (no pun intended) making search.js an UMD file. That's a jQuery UI change, though. This one would be useful to test locally to make sure it really works...

@mgol
Copy link
Member Author

mgol commented Jul 29, 2024

Actually, I could test it via a simple:

npx http-server -p 7001 dist/wordpress 

and opening http://localhost:7001/resources/demos/autocomplete/multiple-remote.html in a browser. PR incoming...

mgol added a commit to mgol/jquery-ui that referenced this pull request Jul 29, 2024
The `jqueryui.com` demos build process deAMDifies HTML files, replacing required
JS files with direct script tags. On the other hand, when running demos locally
from the jQuery UI repository, RequireJS is used.

This used to work fine until we got a new `search.js` file introduced in
jquerygh-2187. The deAMDifying process doesn't touch non-HTML files which made loading
autocomplete demos crash on "require is not a function"

To resolve the issues without a major rearchitecture of the build process,
the `search.js` file now detects AMD and use `require`, falling back to relying
on the `jQuery` global in the other case.

Ref jquerygh-2187
Ref jquery/jqueryui.com#216
@mgol
Copy link
Member Author

mgol commented Jul 29, 2024

The jQuery UI fix is available at jquery/jquery-ui#2274. I tested both the in-repo setup and the jqueryui.com one.

@mgol mgol requested a review from fnagel July 29, 2024 22:10
mgol added a commit to jquery/jquery-ui that referenced this pull request Jul 30, 2024
The `jqueryui.com` demos build process deAMDifies HTML files, replacing required
JS files with direct script tags. On the other hand, when running demos locally
from the jQuery UI repository, RequireJS is used.

This used to work fine until we got a new `search.js` file introduced in
gh-2187. The deAMDifying process doesn't touch non-HTML files which made loading
autocomplete demos crash on "require is not a function"

To resolve the issues without a major rearchitecture of the build process,
the `search.js` file now detects AMD and uses `require`, falling back to relying
on the `jQuery` global in the other case.

Closes gh-2274
Ref gh-2187
Ref jquery/jqueryui.com#216
@mgol mgol self-assigned this Jul 30, 2024
@mgol mgol removed the Needs review label Jul 30, 2024
@mgol mgol merged commit 4ce1b1d into jquery:main Jul 30, 2024
1 check passed
@mgol mgol deleted the demos-fixes branch July 30, 2024 06:36
@mgol mgol removed request for fnagel and timmywil July 30, 2024 06:36
@mgol
Copy link
Member Author

mgol commented Jul 30, 2024

As expected, https://stage.jqueryui.com/autocomplete/#multiple-remote now loads search.js but throws the require is not defined error. I checked that if I set up a local override for search.js, replacing it with what's on jQuery UI main, the demo starts working.

It should be working in production once jQuery UI 1.14.0 is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants