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.

Sign up for GitHub

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

Jump to bottom

Detect AMD loader in prelude to differentiate node require from AMD require #2781

Closed
wants to merge 7 commits into from
Closed

Detect AMD loader in prelude to differentiate node require from AMD require #2781

wants to merge 7 commits into from

Conversation

klipstein
Copy link

↪️ Pull Request

Fixes loading dynamic imports, when an AMD loader (like requirejs) is present on a page before a parcel bundle with dynamic imports got loaded. Currently when requirejs was loaded before loading a parcel bundle with dynamic imports it fails with:

Uncaught Error: Module name "DYNAMIC-IMPORT-FILE.js" has not been loaded yet for context: _. Use require([])

💻 Examples

<!-- index.html -->
<!DOCTYPE html>
<html lang="en">
  <head>
  <title>Parcel + RequireJS</title>
  <script>
        // prevent parcel picking up this script
        document.write('<script src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.6/require.min.js"><\x2fscript>');
      </script>
  </head>
  <body>
     <script>import('./DYNAMIC-IMPORT-FILE').then(() => {});</script>
  </body>
</html>
// DYNAMIC-IMPORT-FILE.js
console.log('works');

🚨 Test instructions

With the above example and bundling the index.html you'll see the mentioned error.

✔️ PR Todo

  • Added/updated unit tests for this change (Note: I was not able to find browser tests within the parcel repo)
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs (Note: did not find any)

@klipstein
Copy link
Author

I tried to get the CI green, but it seems only node_6_x is currently failing.

@klipstein
Copy link
Author

klipstein commented Mar 20, 2019 •
edited

Just wondered why there was no comment on this change yet and I realized that the test-example is not working completely, because dynamic imports are not allowed in HTML documents yet.

To mimic the issue I've create the following files:

index.html:

<!-- index.html -->
<!DOCTYPE html>
<html lang="en">
  <head>
  <title>Parcel + RequireJS</title>
  <script>
        // prevent parcel picking up this script
        document.write('<script src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.6/require.min.js"><\x2fscript>');
      </script>
  </head>
  <body>
     <script src="index.js"></script>
  </body>
</html>

index.js:

import('./dynamic_import').then(() => {});

dynamic_import.js:

console.log('works');

When I now execute parcel index.html and open http://localhost:1234/index.html it show me the following error message and not a works console log:

Uncaught Error: Module name "dynamic_import.js" has not been loaded yet for context: _. Use require([])
...
Uncaught Error: Mismatched anonymous define() module: function () {
       return mainExports;
     }

@klipstein
Copy link
Author

The node 6 tests failed for me before. Does that relate to my change?

@mischnic
Copy link
Member

The node 6 tests failed for me before. Does that relate to my change?

No, we can't figure out why the Windows tests sometimes fail without any error message...

@klipstein
Copy link
Author

Is this pull mergeable now or do I need to adjust something?

@klipstein
Copy link
Author

The tests went through now. Is there something to change to get this merged?

@klipstein
Copy link
Author

Closing it because it was not merged for 6 months.

@klipstein klipstein closed this Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@klipstein @mischnic