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

WIP: add dev-server-hmr #685

Merged
merged 34 commits into from
Nov 2, 2020
Merged

WIP: add dev-server-hmr #685

merged 34 commits into from
Nov 2, 2020

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Oct 4, 2020

if anyone does this before i do or better than i do, we can throw this away.

im just having a go and seeing what i end up with.

cc @LarsDenBakker


TODO:

  • somehow detect dependency trees (scan each file for imports using a parser/ast? 🤔 )
  • write the client part of the code

this is heavily inspired by fred's snowpack work and his esm-hmr repo. its clearly not done but here it is to show you where i was heading.

@changeset-bot
Copy link

changeset-bot bot commented Oct 4, 2020

⚠️ No Changeset found

Latest commit: 363b3ee

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@LarsDenBakker
Copy link
Member

We already scan for module imports, you could use the transformImport hook to build up the dependency graph. You can return nothing from this hook, just use it for analysis.

I recommend https://www.npmjs.com/package/dependency-graph to build a graph, I'm using it in the test runner as well.

@43081j 43081j force-pushed the hmr branch 2 times, most recently from 2fa8ffd to 544233c Compare October 4, 2020 16:25
@43081j
Copy link
Contributor Author

43081j commented Oct 4, 2020

small update:

i got it all running and hooked up now server-side at least. added a bunch of debug logging and have been stepping through it locally.

currently im stuck getting the file watcher to actually watch. i had watch: true set in my config but my event handlers never got hit, so not sure whats up with that.

p.s. the generate tsconfigs script re-ordered the file, if you wondered whats going on with the root tsconfig.json...

@daKmoR
Copy link
Member

daKmoR commented Oct 5, 2020

I'm curious will this - if finished work out of the box with lit-elements idea?

PolymerLabs/hot-elements#1
lit/lit-element#802

@43081j
Copy link
Contributor Author

43081j commented Oct 5, 2020

when talking to lars, the idea was that we should ship a base implementation where users can at least consume the HMR API in their client code (import.meta.hot).

then when peter finishes with his lit implementation we can automatically hook that in for lit consumers.

@FredKSchott
Copy link

Looks great!

Would love to hear where you all matched the esm-hmr spec, and where you all differentiated We improved the spec a bunch when vite integrated it, would love to do the same based on your learnings. Can you create an issue on the repo detailing? https://github.com/pikapkg/esm-hmr

Also since this is based on esm-hmr code, please license properly: https://github.com/pikapkg/esm-hmr/blob/master/LICENSE

@LarsDenBakker
Copy link
Member

modern-web is licensed MIT as well, so that should be covered right?

@FredKSchott
Copy link

you can read the MIT license itself to understand what's required, but basically MIT means that the code is free to use however you like as long as you include the license of the original code in any projects that uses it. Including the license is the only requirement.

@43081j
Copy link
Contributor Author

43081j commented Oct 5, 2020

in fact i didn't base it on esm-hmr so much in the end after all. so im not sure there's a lot to feedback in regards to the spec. i did however make use of similar models (so we should check the licensing i suppose).

the esm-hmr repo doesn't exactly define a "spec" yet as such. it defines some of what the client should do, while defining nothing the server should do. I haven't yet done any of the clientside code for this.

when i get around to implementing the client side of this i will open issues in your repo if there is anything we end up diverging on.

@43081j
Copy link
Contributor Author

43081j commented Oct 11, 2020

just an update:

the client looks to be implemented now. i haven't thoroughly tested it but i have seen it handle an update just fine and re-import the specified module.

couple of things im stuck with:

  • transformImport is passed a source which is a relative path, how can we resolve this to be an absolute client path? as i need the full path for tracking (i.e. its relative to the web server not the filesystem, just like context.path)
  • i moved the logic to append html to the document into a util (appendHtmlToDocument), is there somewhere more sensible for this to belong? so its obvious its a transform. it is used by the web socket plugin too
  • the client is one big blob of JS in a string, is there any possible way we can write it as typescript and instead load the built file?

@LarsDenBakker
Copy link
Member

You should be able to do something along the lines of path.posix.join(path.posix.dirname('/x/y/z/a.js'), '../foo.js'); to create the final browser path.

I want to create a generic HTML maniplation library, but that's something for later.

You can write it as TS, and then read that file as a string using fs and inject that.

@43081j
Copy link
Contributor Author

43081j commented Oct 18, 2020

i rebased onto master and did any remaining pr comments.

still need to write some tests and try it out more thoroughly.

also tried to use parse5-utils but its types seem a bit weird, irregular exports so the ci build fails

@43081j 43081j marked this pull request as ready for review October 31, 2020 18:31
@43081j
Copy link
Contributor Author

43081j commented Oct 31, 2020

this now has some kinda docs and a demo which seems to work.

i tried the demo, changed the css and saw it update. so all seems fine

i would like if one of you can try this out before we merge, though. if anyone is up for that

it isn't needed to enable the file watcher
a less hacky hack
@LarsDenBakker LarsDenBakker merged commit fe3ec35 into modernweb-dev:master Nov 2, 2020
@43081j 43081j deleted the hmr branch November 2, 2020 20:04
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.

4 participants