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

issue 429 javascript module imports #465

Merged

Conversation

hutchgrant
Copy link
Member

Related Issue

#429

Summary

This is a test PR demonstrating the issue related to javascript module imports of unsupported libraries

Problems

  • problems related to javascript library imports for unsupported libraries e.g. redux, lodash, (for this example)

lodash

Uncaught (in promise) SyntaxError: The requested module 'blob:http://localhost:1984/ec43201d-db76-4dd3-943a-02002e7c8fb9' does not provide an export named 'default'
async function (async)
topLevelLoad @ es-module-shims.js:252
processScripts @ es-module-shims.js:453
(anonymous) @ es-module-shims.js:440
(anonymous) @ es-module-shims.js:472

redux - related to a transform path?

es-module-shims.js:409 Uncaught (in promise) Error: Unknown Content-Type ""
    at es-module-shims.js:409

@thescientist13 thescientist13 self-assigned this Jan 7, 2021
@thescientist13 thescientist13 added the help wanted Extra attention is needed label Jan 7, 2021
@thescientist13 thescientist13 linked an issue Jan 21, 2021 that may be closed by this pull request
5 tasks
@thescientist13 thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) CLI P0 Critical issue that should get addressed ASAP and removed enhancement Improve something existing (e.g. no docs, new APIs, etc) labels Jan 23, 2021
@thescientist13 thescientist13 force-pushed the task/issue-429-javascript-module-imports branch from 1d23511 to bb62757 Compare January 23, 2021 22:36
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and recommitted changes on top of new Resource based plugins API, and have been able to get redux and pwa-helpers working so far.

It looks like for lit-redux-router, I "might" have to make the importMap generation recursive to handle deps of deps, but so far making OK progress in this minefield that is multiple module formats on NodeJS. 😳

@thescientist13
Copy link
Member

thescientist13 commented Jan 26, 2021

Latest status (will be continuously updating this comment) but it's pretty much almost there!

Summary

  1. Adding support for more types of npm packages like redux, pwa-helpers, and lit-redux-router
  2. Handle .mjs entry points by extending pluginNodeModules with additional Resource lifecycles
  3. Refactored importMap to build up from transitive dependencies of the project
  4. Created a standalone plugins-import-commonjs plugin that is now integrated and encapsulating all CommonJS related logic. Using a bit of hack in regards to CJS detection though, but. it is working and not interferring adversely with ESM / MJS (yet) so 🤞
  5. Refactored all ResourceInterface methods to be async
  6. Refactored shouldOptimize / optimize to follow pattern of using url and body, but expects a body in return
  7. Added new test cases

TODO

  1. Continue testing with packages like lodash and apollo
  2. Tests
  3. Documentation
  4. Resolve "empty" importMap entry (nice to have)
    "": "/node_modules/",
  5. clean up console logs / test.js code
  6. Open an issue with cjs-module-lexer to see if a test / validate method could be supported, so we can undo our try / catch hack

Thoughts

  • cache importMap so it is only built one time when the server starts, not on every request?
  • this is a good exercise / opportunity in measuring our prior dependencies, to see if "lighter" alternatives exist
  • explore support for more module formats / entry detection, e.g. jsnext:main?

@thescientist13 thescientist13 added the Plugins Greenwood Plugins label Jan 28, 2021
@thescientist13 thescientist13 added the RFC Proposal and changes to workflows, architecture, APIs, etc label Jan 28, 2021
@thescientist13
Copy link
Member

Got a bunch of tests in place now, and synced the docs. Things are feeling pretty solid now.

As a side note, I did play around with apollo for a bit, and it definitely seems pretty coupled to React. I think for our data / GraphQL support plugin, we should consider going with an alternative that is more lighter weight and has less tight coupling to anyone particular frontend framework. Thinking we could give urql a try?

Screen Shot 2021-01-28 at 8 55 16 PM

Screen Shot 2021-01-28 at 8 56 33 PM

@thescientist13 thescientist13 removed the help wanted Extra attention is needed label Jan 30, 2021
@thescientist13 thescientist13 marked this pull request as ready for review January 30, 2021 20:56
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to go! 🎉

@thescientist13 thescientist13 merged commit 2fcc25c into release/0.10.0 Feb 6, 2021
@thescientist13 thescientist13 deleted the task/issue-429-javascript-module-imports branch February 6, 2021 16:38
thescientist13 added a commit that referenced this pull request Apr 3, 2021
* apply npm dependencies using latest Resource plugins changes

* fix linting

* misc cleanup

* recursive project package walking and support for extensionaless bare module specifiers in node modules

* refactor acorn module walking to be recursive

* recursive support for node modules and got lit-redux-router working

* fix packages using process for dead code elimination

* comments

* comments

* make rollup a dependency

* refactor shouldServe async and introduce plugin-import-commonjs

* plugin refactoring and fixing tests

* commonjs working in development

* commonjs working for development

* refactor optimize to async

* commonjs working in develop and prod woohoo

* no more empty entry in import map

* document cjs-module-lexar work around

* add test case for plugin-import-commonjs

* test case for importing modules and added redux / .mjs

* testing more module types

* comment code

* add new import-commonjs plugin to table

* sync ResourceInterface docs

* remove test code

* remove commonjs plugin from website config

Co-authored-by: Owen Buckley <[email protected]>
thescientist13 added a commit that referenced this pull request Apr 3, 2021
* apply npm dependencies using latest Resource plugins changes

* fix linting

* misc cleanup

* recursive project package walking and support for extensionaless bare module specifiers in node modules

* refactor acorn module walking to be recursive

* recursive support for node modules and got lit-redux-router working

* fix packages using process for dead code elimination

* comments

* comments

* make rollup a dependency

* refactor shouldServe async and introduce plugin-import-commonjs

* plugin refactoring and fixing tests

* commonjs working in development

* commonjs working for development

* refactor optimize to async

* commonjs working in develop and prod woohoo

* no more empty entry in import map

* document cjs-module-lexar work around

* add test case for plugin-import-commonjs

* test case for importing modules and added redux / .mjs

* testing more module types

* comment code

* add new import-commonjs plugin to table

* sync ResourceInterface docs

* remove test code

* remove commonjs plugin from website config

Co-authored-by: Owen Buckley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI enhancement Improve something existing (e.g. no docs, new APIs, etc) P0 Critical issue that should get addressed ASAP Plugins Greenwood Plugins RFC Proposal and changes to workflows, architecture, APIs, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validate / support resolution of various common packages in node modules
2 participants