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

feat: add TypeScript definitions #256

Merged
merged 17 commits into from
Apr 21, 2023
Merged

feat: add TypeScript definitions #256

merged 17 commits into from
Apr 21, 2023

Conversation

friedjoff
Copy link
Member

And bundle with esbuild.

@friedjoff friedjoff requested a review from oterral April 17, 2023 08:20
@vercel
Copy link

vercel bot commented Apr 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
openlayers-editor ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 19, 2023 7:37am

@friedjoff friedjoff changed the title TypeScript definitions feat: add TypeScript definitions Apr 17, 2023
@oterral
Copy link
Contributor

oterral commented Apr 17, 2023

Nice . I think you can get rid fo webpacks packages using the basic serve functionnality of esbuild to replace webpack-dev-server.

esbuild build/index.js --bundle --outfile=build/bundle.js --loader:.svg=text --minify --sourcemap --watch --serve

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@friedjoff
Copy link
Member Author

Nice . I think you can get rid fo webpacks packages using the basic serve functionnality of esbuild to replace webpack-dev-server.

esbuild build/index.js --bundle --outfile=build/bundle.js --loader:.svg=text --minify --sourcemap --watch --serve

Good idea, I've tried, but Cypress doesn't seem happy yet.

"not op_mini all",
"not ie <= 11",
"not android < 5"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

I will leave browserslist, because it's important in mapset .... or maybe not something to test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know how to test this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will test and will addd them again if needed

@oterral
Copy link
Contributor

oterral commented Apr 17, 2023

Good idea, I've tried, but Cypress doesn't seem happy yet.

There is always someone unhappy with progress :-D

package.json Outdated Show resolved Hide resolved
tasks/prepare-package.mjs Outdated Show resolved Hide resolved
@oterral
Copy link
Contributor

oterral commented Apr 18, 2023

@friedjoff for the node bug on test. Upgrade the .nvmrc file and use it in the action like this:

https://github.com/geops/trafimage-maps/blob/master/.github/workflows/cypress-chrome.yml#L12

@friedjoff
Copy link
Member Author

for the node bug on test. Upgrade the .nvmrc file and use it in the action like this:

For apps I agree with using something like .nvmrc, but for open source libraries we should not pin to a specific Node.js version to catch incompatibilities with different versions. I've changed the matrix strategy for our Cypress GitHub Action to use the oldest maintained LTS release and the latest stable release to make sure we support a sensible range of Node.js releases.

@oterral
Copy link
Contributor

oterral commented Apr 21, 2023

but for open source libraries we should not pin to a specific Node.js version to catch incompatibilities with different versions

If you pin everybody will use the same node version so no more problem. Dependency like node-canvas are already node version dependent. The only problem I had with node version is when they were not pinned so CI didn't use the good node version and test failed for a month, and nobody care.

@oterral
Copy link
Contributor

oterral commented Apr 21, 2023

Very nice thanks

@oterral oterral merged commit f6db2f6 into master Apr 21, 2023
@oterral oterral deleted the build-ts-definitions branch April 21, 2023 12:58
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